Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements in linear #1091

Merged
merged 10 commits into from
Feb 8, 2022
Merged

Improvements in linear #1091

merged 10 commits into from
Feb 8, 2022

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 self-assigned this Feb 7, 2022
@dellaert dellaert added the quick-review Quick and easy PR to review label Feb 7, 2022
Copy link
Collaborator

@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.

gtsam/linear/GaussianBayesNet.h Show resolved Hide resolved
gtsam/linear/GaussianConditional.cpp Show resolved Hide resolved
gtsam/linear/GaussianConditional.h Show resolved Hide resolved
gtsam/linear/Sampler.h Show resolved Hide resolved
* VectorValues given = ...;
* auto sample = gbn.sample(given, &rng);
*/
VectorValues sample(VectorValues given, std::mt19937_64* rng) const;
Copy link
Collaborator

@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