Skip to content

Conversation

@JamesMcClung
Copy link
Collaborator

I don't know if this is desirable, but this makes runs more deterministic. It was definitely useful for testing/debugging some BGK stuff.

Not all random number generation in the code uses these generators (see #317 ), and even if it did, there could in principle be RNG dependent on other processes and thus sensitive to race conditions.

Copy link
Contributor

@germasch germasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we talked about, I agree this is a good way to go.

I guess I don't understand why you used a shared_ptr, though. The way the implementation currently works I guess there's a singleton-like generator per process, so anything using it doesn't have to worry about it going away?

Well, maybe it's possible that the singleton one gets destructed first as the process finishes, and then something else that has a pointer to it uses it in its own destructor that ends up running later? Is that the thinking?

@germasch germasch force-pushed the pr/deterministic-seed branch from f353285 to fe0294f Compare July 10, 2024 01:38
@JamesMcClung
Copy link
Collaborator Author

I did this a long time ago and don't remember my reasoning for using shared_ptr, but I was probably just trying to avoid a raw pointer on principle. Would a raw pointer, or even just the non-pointer object, be preferable in this case?

@JamesMcClung JamesMcClung force-pushed the pr/deterministic-seed branch from fe0294f to 2a6269c Compare July 10, 2024 18:41
@germasch
Copy link
Contributor

I agree with avoid raw pointers on principle. And a unique_ptr would clearly be in appropriate here.

I googled a little bit on C++ and "singleton" (I think this pretty much is what this is), and it looks like usually people return a reference. I think that could be a suitable alternative, but it can be inconvenient in that reference member vars aren't assignable, so one needs to make sure to set them in the constructor. Of course, an alternative is to not even keep the reference around.

I guess I have some slight preference to follow the singleton pattern (though I might be missing something in how this doesn't really apply to this situation). But I don't really care -- a shared_ptr is pretty explicit in that it reminds one that the rng is a shared resource, even though no reference counting is actually needed...

@JamesMcClung
Copy link
Collaborator Author

I agree that the current implementation is basically the singleton pattern. I could copy this implementation, although this discussion has convinced me that the best solution is dependency injection, since tests should have more control over the rng than a singleton (or really any global) could provide.

Dependency injection would, of course, necessitate adding a template parameter for every class that needs rng (unless there's a better way I haven't thought of). That's by no means a foreign concept to PSC 😛, so if you agree, I wouldn't mind taking a stab at it. Also, I guess there would still have to be a singleton rng, which would be the default template parameter.

@JamesMcClung JamesMcClung merged commit a476eae into psc-code:main Jul 16, 2024
@JamesMcClung JamesMcClung deleted the pr/deterministic-seed branch July 16, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants