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.

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

Jump to bottom

BUG/ENH: Allow bit generators to supply their own constructor #22014

Merged
merged 2 commits into from Jul 28, 2022
Merged

BUG/ENH: Allow bit generators to supply their own constructor #22014

merged 2 commits into from Jul 28, 2022

Conversation

bashtage
Copy link
Contributor

Allow bit generators to supply their own constructors to enable Generator
objects using arbitrary bit generators to be supported

closes #22012

@bashtage
BUG/ENH: ALlow bit generators to supply their own ctor
95e3e7f 
Allow bit generators to supply their own constructors to enable Generator
objects using arbitrary bit generators to be supported

closes numpy#22012
@bashtage
Copy link
Contributor Author

Simplest method I could find to work through #22012. Changes how Generator and RandomState are pickled, but the modified constructor is fully backward compatible so that older pickles can still be used with current NumPy.

@bashtage bashtage mentioned this pull request Jul 19, 2022
Closed
mattip
mattip reviewed Jul 19, 2022
View reviewed changes
numpy/random/tests/test_generator_mt19937.py Outdated
def test_generator_ctor_old_style_pickle():
rg = np.random.Generator(np.random.PCG64DXSM(0))
rg.standard_normal(1)
# Directly call reduce which is used in pickline
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Directly call reduce which is used in pickline
# Directly call reduce which is used in pickling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

mattip
mattip reviewed Jul 19, 2022
View reviewed changes
numpy/random/tests/test_randomstate.py Outdated
def test_randomstate_ctor_old_style_pickle():
rs = np.random.RandomState(MT19937(0))
rs.standard_normal(1)
# Directly call reduce which is used in pickline
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Directly call reduce which is used in pickline
# Directly call reduce which is used in pickling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

mattip
mattip reviewed Jul 19, 2022
View reviewed changes
numpy/random/_pickle.py
@@ -14,70 +14,67 @@
}
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if we could refactor this to avoid the circular import _pickle -> _generator/mtrand -> _pickle

Copy link
Member

Choose a reason for hiding this comment

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

well, not really circular since _generator/mtrand import _pickle in the __reduce__ method, but the pattern is convoluted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the only way to avoid would be to move these to be closed to the class definitions. There is always something circular required here since the main class needs its constructor, and the constructor needs the main class.

Copy link
Member

Choose a reason for hiding this comment

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

That's very common in __reduce__ implementations, IME.

@mattip
Copy link
Member

mattip commented Jul 26, 2022

This LGTM. Any more thoughts?

@mattip mattip merged commit 9180651 into numpy:main Jul 28, 2022
@mattip
Copy link
Member

mattip commented Jul 28, 2022

Thanks @bashtage

@mattip
Copy link
Member

mattip commented Jul 28, 2022

Could you add a release note and mention this somewhere in the documentation?

@bashtage bashtage deleted the change-reduce branch August 3, 2022 22:24
@bashtage
Copy link
Contributor Author

@mattip I was thinking of writing the release note, but I'm not sure there is really anything worth saying. This changes the pickling information in Generator and RandomState but in a backward compatible way. Did you see anything that is user-relevant?

@mattip
Copy link
Member

mattip commented Aug 14, 2022 •
edited

Maybe something like this in a release note?

Allow pickling/unpickling arbitrary random ``BitGenerators``. Previously, code like this would fail:

    g = np.random.Generator(rg.MT64())
    pickle.loads(pickle.dumps(g))

@bashtage bashtage mentioned this pull request Aug 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkern rkern rkern left review comments

@mattip mattip mattip left review comments

Assignees
No one assigned
Labels
00 - Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: np.random.Generator cannot be unpickled using 3rd party bit generators
3 participants
@bashtage @mattip @rkern