Skip to content

[fix] RNG arguments for distribution samplers#822

Merged
astroJeff merged 8 commits intomainfrom
sg_rng_args
Mar 17, 2026
Merged

[fix] RNG arguments for distribution samplers#822
astroJeff merged 8 commits intomainfrom
sg_rng_args

Conversation

@sgossage
Copy link
Contributor

@sgossage sgossage commented Mar 16, 2026

This PR contains a bug fix taken from PR#797 that addresses the same issue.

We have the option to fix the RNG seed (setting a fixed entropy), but any RNG instances with a fixed seed are not passed to several functions, meaning that orbital period and formation times can not be generated with a fixed seed in the intended way. This gives the following functions an rng argument to fix that:

  • posydon.utils.rejection_sampler
  • posydon.utils.inverse_sampler
  • posydon.utils.histogram_sampler

In versions including and earlier than v2.2.6, This would affect any attempt to fix the RNG seed when the SFH scenario is not burst and when the orbital_period_scheme is 'Sana+12_period_extended' or Moe and DiStefano 17.

Warning

This bumps main to v2.2.7

@sgossage sgossage marked this pull request as ready for review March 16, 2026 16:44
@sgossage sgossage requested a review from a team March 16, 2026 16:44
@sgossage sgossage self-assigned this Mar 16, 2026
Copy link
Contributor

@astroJeff astroJeff left a comment

Choose a reason for hiding this comment

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

Everything looks good to me

@astroJeff
Copy link
Contributor

I approved, but note, there are a few other places where np.random is used:

  1. In the GRB beaming factor calculation (I am not sure if this is still used)
  2. In the Bondi-Hoyle accretion rate (when calculating the orbital phase for solving Kepler's Eqn. This is, I think, an incorrect way to approach the problem - we should be calculating a secular average, but we should look into it further)
  3. In a bunch of the interpolation and classification training scripts
  4. In the Moe & Di Stefano initial distribution

@sgossage this last one, in particular, worries me. Can you look into it? I think it should be a quick change that can be added to this PR

@sgossage
Copy link
Contributor Author

sgossage commented Mar 16, 2026

thanks @astroJeff , I've updated the Moe & Di Stefano sampler now with the same fix.

Looking at bondi_hoyle, I think doing a secular average would be good (in which case we could potentially avoid np.random in these calls), but I'm noticing anyways that we probably want to pass any existing (potentially fixed seed) RNGs to the evolution steps too.

step_SN also uses np.random functions, which could be fixed this way. It would also give flexibility for any additional random functions in the future. I think we could fix draws from the Maxwellian (and lognorm) distribution to get reproducible kick draws -- potentially replacing the SciPy stats functions with NumPy equivalents. This will take a bit of retooling though.

For the training/interpolation, we could allow the same flexibility to be consistent. Those cases seem like np.random is used for cross validation, which probably shouldn't be controlled by entropy (?).

The GRB function is not used anywhere in the code, but could be called by a user -- we can add the ability to pass and RNG.

Copy link
Collaborator

@maxbriel maxbriel left a comment

Choose a reason for hiding this comment

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

This looks good to me. Similar to the implementation included in PR #797, except more focused and into main.

Only one line that can be removed before accepting.

Co-authored-by: Max <14039563+maxbriel@users.noreply.github.com>
@astroJeff astroJeff merged commit e38d6e8 into main Mar 17, 2026
4 checks passed
@astroJeff astroJeff deleted the sg_rng_args branch March 17, 2026 16:15
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.

3 participants