Skip to content

Support NumPy 2 & enable ruff NPY warnings #126

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

luator
Copy link
Member

@luator luator commented Oct 28, 2024

Not too long ago, NumPy 2.0 got released. cluster_utils didn't work with it out of the box, so we restricted the dependency to numpy<2.

Looking into it now, it seems the only actual problem was in the Discrete distribution when using booleans: Using NumPy's choice() converted the list of native bools to an array of np.bool. Later one, this clashed when passing the parameters to the job script, where they are parsed with ast.literal_eval(). Simply converting back to list with .tolist() seems to be enough to fix this.

I also enabled the NPY warnings of ruff, which can aid migration. The only warnings it showed where related to usage of np.random (it's preferred to use a different rng nowadays), though. While this might not be an issue for Numpy 2, I still changed to using a random generator created with np.random.default_rng() and added a utility function to easily access it thoughout the project.

Closes #112

@luator luator self-assigned this Oct 28, 2024
@luator luator requested a review from nitinjain96 October 31, 2024 15:41
@luator luator added the enhancement New feature or request label Oct 31, 2024
luator added 4 commits April 15, 2025 09:29
Fixing those might bring us forward on the path to Numpy 2.0
compatibility.
The recommended way is not to create a random generator instance (e.g.
via `np.random.default_rng()` and use that.  It is supposed to be
significantly faster and statistically more robust.

In case we ever want to be able to set a seed for cluster_utils itself,
it's probably best to have one global generator used throughout the
package.  Add this, together with a little getter function `get_rng()`
in base/utils.py.

This fixes ruffs NPY warnings.
Show a more informative error message when the parameter dict, passed to
the job script, cannot be parsed by `ast.literal_eval`.  It now includes
the string to be parsed itself, to make debugging easier.

Since the string, that is passed here, is constructed by cluster_utils
when used normally, an error here most likely indicates a bug in
cluster_utils rather than a user mistake.
Not too long ago, NumPy 2.0 got released.  cluster_utils didn't work
with it out of the box, so we restricted the dependency to numpy<2.

Looking into it now, it seems the only actual problem was in the
`Discrete` distribution when using booleans:  Using NumPy's `choice()`
converted the list of native bools to an array of np.bool.  Later one,
this clashed when passing the parameters to the job script, where they
are parsed with `ast.literal_eval()`.
Simply converting back to list with `.tolist()` seems to be enough to
fix this.
@luator
Copy link
Member Author

luator commented Apr 15, 2025

I noticed that for some reasons the checks are not running for this PR. I'll close and reopen in the hope that this will trigger the checks.

@luator luator closed this Apr 15, 2025
@luator
Copy link
Member Author

luator commented Apr 15, 2025

I noticed that for some reasons the checks are not running for this PR. I'll close and reopen in the hope that this will trigger the checks.

@luator luator reopened this Apr 15, 2025
@luator
Copy link
Member Author

luator commented Apr 15, 2025

Seems it worked :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NumPy 2.0
1 participant