Skip to content

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

Merged
mattip merged 2 commits intonumpy:mainfrom
bashtage:change-reduce
Jul 28, 2022
Merged

BUG/ENH: Allow bit generators to supply their own constructor#22014
mattip merged 2 commits intonumpy:mainfrom
bashtage:change-reduce

Conversation

@bashtage
Copy link
Copy Markdown
Contributor

@bashtage bashtage commented Jul 19, 2022

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
Copy Markdown
Contributor Author

bashtage commented Jul 19, 2022

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
Copy Markdown
Member

@mattip mattip Jul 19, 2022

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
Copy Markdown
Contributor Author

@bashtage bashtage Jul 19, 2022

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
Copy Markdown
Member

@mattip mattip Jul 19, 2022

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
Copy Markdown
Contributor Author

@bashtage bashtage Jul 19, 2022

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
Copy Markdown
Member

@mattip mattip Jul 19, 2022

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
Copy Markdown
Member

@mattip mattip Jul 19, 2022

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
Copy Markdown
Contributor Author

@bashtage bashtage Jul 19, 2022

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
Copy Markdown
Member

@rkern rkern Jul 19, 2022

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
Copy Markdown
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
Copy Markdown
Member

mattip commented Jul 28, 2022

Thanks @bashtage

@mattip
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

bashtage commented Aug 11, 2022

@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
Copy Markdown
Member

mattip commented Aug 14, 2022 •
edited
Loading

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