Conversation
dellaert
left a comment
dellaert
left a comment
There was a problem hiding this comment.
An improvement, but maybe inconsistent now between various files? I think it should all be nullptr default or all something else, no? And we should clarify if the default will then be a file-specific local rng or a global rng.
| * @return HybridValues | ||
| */ | ||
| HybridValues sample(const HybridValues &given, std::mt19937_64 *rng) const; | ||
| HybridValues sample(const HybridValues &given, |
There was a problem hiding this comment.
What happens when nullptr? Seems different from yesterday's PR where we pass in a kSomething
There was a problem hiding this comment.
Passing in a static variable address as a default argument is
- Not idiomatic C++ which recommends using a
nullptras the default for a raw pointer. - Did not play well with the python wrapper due to memory allocation.
Whenever a sample method gets a nullptr for the rng, it keeps passing it along until we actually use the rng (e.g. in Sample::sample). At that point, I have a simple check:
rng = (rng == nullptr) ? &kRandomNumberGenerator : rng;This makes the code really clean since we don't have to worry about the rng pointer until we actually need it.
There was a problem hiding this comment.
Ok. And yesterday’s changes are now also defaulting to null ptr?
There was a problem hiding this comment.
Yes, I needed to do that to get the CI passing.
| Key key = firstFrontalKey(); | ||
|
|
||
| // Check if rng is nullptr, then assign default | ||
| rng = (rng == nullptr) ? &kRandomNumberGenerator : rng; |
There was a problem hiding this comment.
Is this a local or a global default rng?
There was a problem hiding this comment.
It's not a global default. There is one definition in GaussianConditional.cpp and one in DiscreteConditional.h. I think we should make it a global default so code like ShonanAveraging can also leverage it (they all use the same seed of 42).
|
Rather than renaming, (i don’t like the new name so much) you can put each one in a cpp file in an anonymous namespace |
|
I was hoping to follow your suggestion, perhaps in another PR. |
2 participants
Using lessons learned in #2136, I've updated the sampling methods to use default parameters for the PRNG. This not only reduces code duplication, but is also fully backwards-compatible.
I use
nullptras the default parameter since that is idiomatic in C++.