Skip to content

Update sample methods #2137

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.

Sign up for GitHub

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

Merged
dellaert merged 8 commits into from
May 19, 2025
Merged

Update sample methods #2137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
95af327
make sample methods DRY in HybridBayesNet and GaussianConditional
varunagrawal May 16, 2025
d4c9c68
update wrapper with rng default args
varunagrawal May 16, 2025
bc7646f
add global PRNG to utilities.h
varunagrawal May 17, 2025
995bedb
use default RNG object for mt19937_64 everywhere
varunagrawal May 17, 2025
8302f2d
use global kRandomNumberGenerator
varunagrawal May 17, 2025
12baf54
rename kRandomNumberGenerator in local files to kPRNG to prevent coll…
varunagrawal May 17, 2025
4b6af3d
remove more collisions
varunagrawal May 17, 2025
618505e
separate checks for Apple and Windows since the RNG values are different
varunagrawal May 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions gtsam/base/utilities.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
#pragma once

#include <string>
#include <gtsam/dllexport.h>

#include <iostream>
#include <random>
#include <sstream>
#include <string>

#include <gtsam/dllexport.h>
/**
* @brief Global default pseudo-random number generator object.
* In wrappers we can access std::mt19937_64 via gtsam.MT19937
*/
static std::mt19937_64 kRandomNumberGenerator(42);

namespace gtsam {
/**
Expand All @@ -28,7 +35,7 @@ struct GTSAM_EXPORT RedirectCout {
std::streambuf* coutBuffer_;
};

}
} // namespace gtsam

namespace gtsam {
// Adapted from https://stackoverflow.com/a/32223343/9151520
Expand Down
1 change: 1 addition & 0 deletions gtsam/discrete/DiscreteConditional.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#include <gtsam/base/Testable.h>
#include <gtsam/base/debug.h>
#include <gtsam/base/utilities.h>
#include <gtsam/discrete/DiscreteConditional.h>
#include <gtsam/discrete/Ring.h>
#include <gtsam/discrete/Signature.h>
Expand Down
3 changes: 0 additions & 3 deletions gtsam/discrete/DiscreteConditional.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@
#include <string>
#include <vector>

// In wrappers we can access std::mt19937_64 via gtsam.MT19937
static std::mt19937_64 kRandomNumberGenerator(42);

namespace gtsam {

/**
Expand Down
1 change: 1 addition & 0 deletions gtsam/discrete/TableDistribution.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include <gtsam/base/Testable.h>
#include <gtsam/base/debug.h>
#include <gtsam/base/utilities.h>
#include <gtsam/discrete/Ring.h>
#include <gtsam/discrete/Signature.h>
#include <gtsam/discrete/TableDistribution.h>
Expand Down
10 changes: 0 additions & 10 deletions gtsam/hybrid/HybridBayesNet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,16 +202,6 @@ HybridValues HybridBayesNet::sample(std::mt19937_64 *rng) const {
return sample(given, rng);
}

/* ************************************************************************* */
HybridValues HybridBayesNet::sample(const HybridValues &given) const {
return sample(given, &kRandomNumberGenerator);
}

/* ************************************************************************* */
HybridValues HybridBayesNet::sample() const {
return sample(&kRandomNumberGenerator);
}

/* ************************************************************************* */
AlgebraicDecisionTree<Key> HybridBayesNet::errorTree(
const VectorValues &continuousValues) const {
Expand Down
24 changes: 5 additions & 19 deletions gtsam/hybrid/HybridBayesNet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,11 @@ class GTSAM_EXPORT HybridBayesNet : public BayesNet<HybridConditional> {
* auto sample = bn.sample(given, &rng);
*
* @param given Values of missing variables.
* @param rng The pseudo-random number generator.
* @param rng The optional pseudo-random number generator.
* @return HybridValues
*/
HybridValues sample(const HybridValues &given, std::mt19937_64 *rng) const;
HybridValues sample(const HybridValues &given,
Copy link
Copy Markdown
Member

@dellaert dellaert May 16, 2025

Choose a reason for hiding this comment

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

What happens when nullptr? Seems different from yesterday's PR where we pass in a kSomething

Copy link
Copy Markdown
Contributor Author

@varunagrawal varunagrawal May 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Passing in a static variable address as a default argument is

  1. Not idiomatic C++ which recommends using a nullptr as the default for a raw pointer.
  2. 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.

Copy link
Copy Markdown
Member

@dellaert dellaert May 17, 2025

Choose a reason for hiding this comment

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

Ok. And yesterday’s changes are now also defaulting to null ptr?

Copy link
Copy Markdown
Contributor Author

@varunagrawal varunagrawal May 17, 2025

Choose a reason for hiding this comment

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

Yes, I needed to do that to get the CI passing.

std::mt19937_64 *rng = nullptr) const;

/**
* @brief Sample using ancestral sampling.
Expand All @@ -193,25 +194,10 @@ class GTSAM_EXPORT HybridBayesNet : public BayesNet<HybridConditional> {
* std::mt19937_64 rng(42);
* auto sample = bn.sample(&rng);
*
* @param rng The pseudo-random number generator.
* @param rng The optional pseudo-random number generator.
* @return HybridValues
*/
HybridValues sample(std::mt19937_64 *rng) const;

/**
* @brief Sample from an incomplete BayesNet, use default rng.
*
* @param given Values of missing variables.
* @return HybridValues
*/
HybridValues sample(const HybridValues &given) const;

/**
* @brief Sample using ancestral sampling, use default rng.
*
* @return HybridValues
*/
HybridValues sample() const;
HybridValues sample(std::mt19937_64 *rng = nullptr) const;

/**
* @brief Prune the Bayes Net such that we have at most maxNrLeaves leaves.
Expand Down
6 changes: 2 additions & 4 deletions gtsam/hybrid/hybrid.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,8 @@ class HybridBayesNet {
gtsam::HybridValues optimize() const;
gtsam::VectorValues optimize(const gtsam::DiscreteValues& assignment) const;

gtsam::HybridValues sample(const gtsam::HybridValues& given, std::mt19937_64@ rng) const;
gtsam::HybridValues sample(std::mt19937_64@ rng) const;
gtsam::HybridValues sample(const gtsam::HybridValues& given) const;
gtsam::HybridValues sample() const;
gtsam::HybridValues sample(const gtsam::HybridValues& given, std::mt19937_64@ rng = nullptr) const;
gtsam::HybridValues sample(std::mt19937_64@ rng = nullptr) const;

void print(string s = "HybridBayesNet\n",
const gtsam::KeyFormatter& keyFormatter =
Expand Down
12 changes: 0 additions & 12 deletions gtsam/linear/GaussianBayesNet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@
using namespace std;
using namespace gtsam;

// In Wrappers we have no access to this so have a default ready
static std::mt19937_64 kRandomNumberGenerator(42);

namespace gtsam {

// Instantiate base class
Expand Down Expand Up @@ -76,15 +73,6 @@ namespace gtsam {
return result;
}

/* ************************************************************************ */
VectorValues GaussianBayesNet::sample() const {
return sample(&kRandomNumberGenerator);
}

VectorValues GaussianBayesNet::sample(const VectorValues& given) const {
return sample(given, &kRandomNumberGenerator);
}

/* ************************************************************************ */
VectorValues GaussianBayesNet::optimizeGradientSearch() const
{
Expand Down
10 changes: 2 additions & 8 deletions gtsam/linear/GaussianBayesNet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ namespace gtsam {
* std::mt19937_64 rng(42);
* auto sample = gbn.sample(&rng);
*/
VectorValues sample(std::mt19937_64* rng) const;
VectorValues sample(std::mt19937_64* rng = nullptr) const;

/**
* Sample from an incomplete BayesNet, given missing variables
Expand All @@ -140,13 +140,7 @@ namespace gtsam {
* VectorValues given = ...;
* auto sample = gbn.sample(given, &rng);
*/
VectorValues sample(const VectorValues& given, std::mt19937_64* rng) const;

/// Sample using ancestral sampling, use default rng
VectorValues sample() const;

/// Sample from an incomplete BayesNet, use default rng
VectorValues sample(const VectorValues& given) const;
VectorValues sample(const VectorValues& given, std::mt19937_64* rng = nullptr) const;

/**
* Return ordering corresponding to a topological sort.
Expand Down
21 changes: 7 additions & 14 deletions gtsam/linear/GaussianConditional.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@
* @author Christian Potthast, Frank Dellaert
*/

#include <gtsam/base/utilities.h>
#include <gtsam/hybrid/HybridValues.h>
#include <gtsam/linear/GaussianConditional.h>
#include <gtsam/linear/Sampler.h>
#include <gtsam/linear/VectorValues.h>
#include <gtsam/linear/linearExceptions.h>
#include <gtsam/hybrid/HybridValues.h>

#ifdef __GNUC__
#pragma GCC diagnostic push
Expand All @@ -34,9 +35,6 @@
#include <string>
#include <cmath>

// In wrappers we can access std::mt19937_64 via gtsam.MT19937
static std::mt19937_64 kRandomNumberGenerator(42);

using namespace std;

namespace gtsam {
Expand Down Expand Up @@ -347,6 +345,10 @@ namespace gtsam {

VectorValues solution = solve(parentsValues);
Key key = firstFrontalKey();

// Check if rng is nullptr, then assign default
rng = (rng == nullptr) ? &kRandomNumberGenerator : rng;
Copy link
Copy Markdown
Member

@dellaert dellaert May 16, 2025

Choose a reason for hiding this comment

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

Is this a local or a global default rng?

Copy link
Copy Markdown
Contributor Author

@varunagrawal varunagrawal May 17, 2025

Choose a reason for hiding this comment

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

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


// The vector of sigma values for sampling.
// If no model, initialize sigmas to 1, else to model sigmas
const Vector& sigmas = (!model_) ? Vector::Ones(rows()) : model_->sigmas();
Expand All @@ -359,16 +361,7 @@ namespace gtsam {
throw std::invalid_argument(
"sample() can only be invoked on no-parent prior");
VectorValues values;
return sample(values);
}

/* ************************************************************************ */
VectorValues GaussianConditional::sample() const {
return sample(&kRandomNumberGenerator);
}

VectorValues GaussianConditional::sample(const VectorValues& given) const {
return sample(given, &kRandomNumberGenerator);
return sample(values, rng);
}

/* ************************************************************************ */
Expand Down
10 changes: 2 additions & 8 deletions gtsam/linear/GaussianConditional.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ namespace gtsam {
* std::mt19937_64 rng(42);
* auto sample = gc.sample(&rng);
*/
VectorValues sample(std::mt19937_64* rng) const;
VectorValues sample(std::mt19937_64* rng = nullptr) const;

/**
* Sample from conditional, given missing variables
Expand All @@ -227,13 +227,7 @@ namespace gtsam {
* auto sample = gc.sample(given, &rng);
*/
VectorValues sample(const VectorValues& parentsValues,
std::mt19937_64* rng) const;

/// Sample, use default rng
VectorValues sample() const;

/// Sample with given values, use default rng
VectorValues sample(const VectorValues& parentsValues) const;
std::mt19937_64* rng = nullptr) const;

/// @}
/// @name Linear algebra.
Expand Down
14 changes: 7 additions & 7 deletions gtsam/linear/linear.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,10 +560,9 @@ virtual class GaussianConditional : gtsam::JacobianFactor {
const gtsam::VectorValues& frontalValues) const;
gtsam::JacobianFactor* likelihood(gtsam::Vector frontal) const;

gtsam::VectorValues sample(std::mt19937_64@ rng) const;
gtsam::VectorValues sample(const gtsam::VectorValues& parents, std::mt19937_64@ rng) const;
gtsam::VectorValues sample() const;
gtsam::VectorValues sample(const gtsam::VectorValues& parents) const;
gtsam::VectorValues sample(std::mt19937_64 @rng = nullptr) const;
gtsam::VectorValues sample(const gtsam::VectorValues& parents,
std::mt19937_64 @rng = nullptr) const;

// Advanced Interface
gtsam::VectorValues solveOtherRHS(const gtsam::VectorValues& parents,
Expand Down Expand Up @@ -629,9 +628,10 @@ virtual class GaussianBayesNet {
gtsam::VectorValues optimize() const;
gtsam::VectorValues optimize(const gtsam::VectorValues& given) const;
gtsam::VectorValues optimizeGradientSearch() const;

gtsam::VectorValues sample(const gtsam::VectorValues& given) const;
gtsam::VectorValues sample() const;

gtsam::VectorValues sample(const gtsam::VectorValues& given,
std::mt19937_64 @rng = nullptr) const;
gtsam::VectorValues sample(std::mt19937_64 @rng = nullptr) const;
gtsam::VectorValues backSubstitute(const gtsam::VectorValues& gx) const;
gtsam::VectorValues backSubstituteTranspose(const gtsam::VectorValues& gx) const;

Expand Down
8 changes: 5 additions & 3 deletions gtsam/linear/tests/testGaussianConditional.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ TEST(GaussianConditional, likelihood) {

/* ************************************************************************* */
// Test sampling
TEST(GaussianConditional, sample) {
TEST(GaussianConditional, Sample) {
Matrix A1 = (Matrix(2, 2) << 1., 2., 3., 4.).finished();
const Vector2 b(20, 40), x1(3, 4);
const double sigma = 0.01;
Expand All @@ -465,8 +465,10 @@ TEST(GaussianConditional, sample) {
auto actual3 = conditional.sample(given, &rng);
EXPECT_LONGS_EQUAL(1, actual2.size());
// regressions
#if __APPLE__ || _WIN32
EXPECT(assert_equal(Vector2(31.0111856, 64.9850775), actual2[X(0)], 1e-5));
#if __APPLE__
EXPECT(assert_equal(Vector2(31.0111856, 64.9850775), actual2[X(0)], 1e-5));
#elif _WIN32
EXPECT(assert_equal(Vector2(30.995317, 64.9943165), actual2[X(0)], 1e-5));
#elif __linux__
EXPECT(assert_equal(Vector2(30.9809331, 64.9927588), actual2[X(0)], 1e-5));
#endif
Expand Down
6 changes: 3 additions & 3 deletions gtsam/sfm/ShonanAveraging.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
namespace gtsam {

// In Wrappers we have no access to this so have a default ready
static std::mt19937 kRandomNumberGenerator(42);
static std::mt19937 kPRNG(42);

using Sparse = Eigen::SparseMatrix<double>;

Expand Down Expand Up @@ -869,7 +869,7 @@ Values ShonanAveraging<d>::initializeRandomly(std::mt19937 &rng) const {
/* ************************************************************************* */
template <size_t d>
Values ShonanAveraging<d>::initializeRandomly() const {
return initializeRandomly(kRandomNumberGenerator);
return initializeRandomly(kPRNG);
}

/* ************************************************************************* */
Expand All @@ -883,7 +883,7 @@ Values ShonanAveraging<d>::initializeRandomlyAt(size_t p,
/* ************************************************************************* */
template <size_t d>
Values ShonanAveraging<d>::initializeRandomlyAt(size_t p) const {
return initializeRandomlyAt(p, kRandomNumberGenerator);
return initializeRandomlyAt(p, kPRNG);
}

/* ************************************************************************* */
Expand Down
4 changes: 2 additions & 2 deletions gtsam/sfm/TranslationRecovery.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ using namespace gtsam;
using namespace std;

// In Wrappers we have no access to this so have a default ready.
static std::mt19937 kRandomNumberGenerator(42);
static std::mt19937 kPRNG(42);

// Some relative translations may be zero. We treat nodes that have a zero
// relativeTranslation as a single node.
Expand Down Expand Up @@ -185,7 +185,7 @@ Values TranslationRecovery::initializeRandomly(
const std::vector<BinaryMeasurement<Point3>> &betweenTranslations,
const Values &initialValues) const {
return initializeRandomly(relativeTranslations, betweenTranslations,
&kRandomNumberGenerator, initialValues);
&kPRNG, initialValues);
}

Values TranslationRecovery::run(
Expand Down
Loading
Loading