Skip to content

Conversation

dellaert
Copy link
Member

@dellaert dellaert commented Feb 7, 2022

Improvements in linear:

  • FromMeanAndStddev constructors
  • sampling using std random number generators
  • wrapping of above
  • documentation improvements

@dellaert dellaert requested a review from varunagrawal February 7, 2022 01:50
@dellaert dellaert self-assigned this Feb 7, 2022
@dellaert dellaert added the quick-review Quick and easy PR to review label Feb 7, 2022
Copy link
Contributor

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

A bunch of comments but LGTM.

* VectorValues given = ...;
* auto sample = gbn.sample(given, &rng);
*/
VectorValues sample(VectorValues given, std::mt19937_64* rng) const;
Copy link
Contributor

@varunagrawal varunagrawal Feb 8, 2022

Choose a reason for hiding this comment

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

Okay, I have a better suggestion. You can change this signature to

VectorValues sample(VectorValues given, std::mt19937_64 rng = std::mt19937_64(42)) const;

and the definition would have us pass &rng. I tried it out locally and this works great!

Con: You will have convert from a pointer to a reference pass-by-value.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sticking to my solution, as it uses one static rng in cpp.
Don't spend too much time on this.

@dellaert dellaert merged commit c344562 into develop Feb 8, 2022
@dellaert dellaert deleted the feature/linear_samplers branch February 8, 2022 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quick-review Quick and easy PR to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants