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

Bump numpy version due to use of Generator.spawn only available in >=1.25 #7607

Merged
merged 4 commits into from
Dec 5, 2024

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Dec 5, 2024

Introduced in #7508 (specifically 4ea1406)

Closes #7605

Actually #7508 had a subtle breaking change, in that it used to be possible to seed each chain separately by passing a list of integers/rngs. Now there's no such control. Hopefully nobody needs that behavior these days...


📚 Documentation preview 📚: https://pymc--7607.org.readthedocs.build/en/7607/

@ricardoV94 ricardoV94 added bug dependencies Pull requests that update a dependency file labels Dec 5, 2024
@ricardoV94 ricardoV94 requested a review from lucianopaz December 5, 2024 08:58
@ricardoV94 ricardoV94 changed the title Do not use recent Generator.spawn for compatibility with older versions of numpy Do not use recent Generator.spawn for compatibility with older versions of numpy Dec 5, 2024
@@ -166,7 +166,7 @@ def __init__(
if scaling is None and potential is None:
mean = floatX(np.zeros(size))
var = floatX(np.ones(size))
potential = QuadPotentialDiagAdapt(size, mean, var, 10, rng=self.rng.spawn(1)[0])
potential = QuadPotentialDiagAdapt(size, mean, var, 10, rng=self.rng)
Copy link
Member Author

Choose a reason for hiding this comment

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

Why was this spawning a new rng?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the potential and the step method have to own independent generators. If they were to share the generator, the sampling state will lose that information, and the step method you’d get from the sampling state would produce different draws than the original step method with a shared generator

Copy link
Member Author

Choose a reason for hiding this comment

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

There is one potential per step, so makes sense to have a single source of randomness. The state of the potential should be a subset of the the state of the step.

Copy link
Contributor

Choose a reason for hiding this comment

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

And I think that you get that with the spawning. Both streams will be independent, but providing the rng to the step method will determine the stream that is produced by the potential

pymc/sampling/mcmc.py Outdated Show resolved Hide resolved
Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

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

Lgtm. I’m not sure if there were any other files that needed the numpy version bump. I suppose that the tests should pick that up.

@ricardoV94 ricardoV94 changed the title Do not use recent Generator.spawn for compatibility with older versions of numpy Bump minimum numpy version due to use of Generator.spawn only available in numpy>1.25 Dec 5, 2024
@ricardoV94 ricardoV94 changed the title Bump minimum numpy version due to use of Generator.spawn only available in numpy>1.25 Bump numpy version due to use of Generator.spawn only available in >=1.25 Dec 5, 2024
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.84%. Comparing base (955ac23) to head (50e034b).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
pymc/sampling/mcmc.py 66.66% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7607      +/-   ##
==========================================
- Coverage   92.85%   92.84%   -0.01%     
==========================================
  Files         106      106              
  Lines       17742    17745       +3     
==========================================
+ Hits        16474    16476       +2     
- Misses       1268     1269       +1     
Files with missing lines Coverage Δ
pymc/sampling/mcmc.py 86.12% <66.66%> (-0.13%) ⬇️

@ricardoV94 ricardoV94 merged commit a714b24 into pymc-devs:main Dec 5, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Sampling fails because 'numpy.random._generator.Generator' object has no attribute 'spawn'
2 participants