-
Notifications
You must be signed in to change notification settings - Fork 54
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
MNT Support numpy2 #429
MNT Support numpy2 #429
Conversation
This reverts commit 3803876.
CI is green. Except that a single line is not tested since we're testing a recent numpy<2 here. I'll have another PR to test minimum dependencies in CI, but for now this fixes urgent issues and I think we should merge and release. (FYI, numpy issues on the skops side @TamaraAtanasoska) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating skops to work with numpy v2 and for adding the other improvements.
res = { | ||
"__class__": obj.__class__.__name__, | ||
"__module__": get_module(type(obj)), | ||
"__loader__": "RandomGeneratorNode", | ||
"content": {"bit_generator": bit_generator_state}, | ||
"content": {"bit_generator": bit_generator_state, "seed_seq": seed_seq_state}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I can't quite remember how it worked: Would adding an item here affect compatibility? I guess it's backwards compatible as witnessed by the test but not forwards compatible? Also, is this a change for numpy v2 or was it an oversight of not having this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in numpy~=1
we have:
>>> np.random.default_rng().__reduce__()
(<function __generator_ctor at 0x777841d798a0>, ('PCG64', <function __bit_generator_ctor at 0x777841d3cc20>), {'bit_generator': 'PCG64', 'state': {'state': 220935942547292961595576198891696034818, 'inc': 281084058663618135801235667263986488477}, 'has_uint32': 0, 'uinteger': 0})
while numpy=2
does this:
>>> np.random.default_rng().__reduce__()
(<function __generator_ctor at 0x778c98b64b80>, (<numpy.random._pcg64.PCG64 object at 0x778c99543480>,), None)
and that means our check fails, and while fixing the check going recursively in what's in the output of __reduce__
, I noticed we're not saving the seed sequence's state. It doesn't seem to have an affect in the next few generated numbers as I checked, but it's something which can be stored and loaded, so I added it.
fixes #425
This adds support for numpy2. In the process, I also found some other issues which I fixed. Some of them were in tests for model cards, and I think the changed code is nicer since it leaves the titles of the sections unchanged.