Support NumPy 2 & enable ruff NPY warnings #126
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
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.
|
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. |
|
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. |
|
Seems it worked :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Discretedistribution when using booleans: Using NumPy'schoice()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 withast.literal_eval(). Simply converting back to list with.tolist()seems to be enough to fix this.I also enabled the
NPYwarnings of ruff, which can aid migration. The only warnings it showed where related to usage ofnp.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 withnp.random.default_rng()and added a utility function to easily access it thoughout the project.Closes #112