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.

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

Couple of numpy-related fixes #140

Merged
merged 3 commits into from
Jan 15, 2025
Merged

Couple of numpy-related fixes #140

merged 3 commits into from
Jan 15, 2025

Conversation

davidwilby
Copy link
Collaborator

📝 Description

We have a few tests routinely failing at the moment for a handful of reasons mostly relating to numpy 2.

One such error is due to DeepSensor's dependency stheno not yet pinning the version of fdm which repairs some of the numpy compatibility bugs. I've raised a PR in that project here: wesselb/stheno#27

In this PR there are a couple of corrections to alleviate test failures:

  1. Use of np.product in some tests, this was changed in something like numpy 1.16 to np.prod.
  2. The type of encoder_scales was being set to a numpy float32 with numpy 2 (possible relating to https://numpy.org/devdocs/numpy_2_0_migration_guide.html#changes-to-numpy-data-type-promotion) which was causing the saving and loading test to fail. I've added an int() but unsure if this is appropriate/ideal, though all of the tests pass locally.

✅ Checklist before requesting a review

(See the contributing guide for more details on these steps.)

  • I have installed developer dependencies with pip install -r requirements/requirements.dev.txt and running pre-commit install (or alternatively, manually running ruff format before commiting)

If changing or adding source code:

  • tests are included and are passing (run pytest).
  • documentation is included or updated as relevant, including docstrings.

Copy link
Collaborator

@tom-andersson tom-andersson left a comment

Choose a reason for hiding this comment

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

Awesome, thanks so much for catching these niche deprecation bugs @davidwilby!

You mentioned changing encoder_scales to an int but I can't see that in this PR. Looks like the tests are passing here, so maybe it's not an issue? I see that numpy 2.2 is being used by the test worker.

encoder_scales has to be a float though, not an int, so we'd have to find another solution if this is an issue.

@tom-andersson
Copy link
Collaborator

@all-contributors please add @davidwilby for bug

Copy link
Contributor

@tom-andersson

I've put up a pull request to add @davidwilby! 🎉

@davidwilby
Copy link
Collaborator Author

You mentioned changing encoder_scales to an int but I can't see that in this PR. Looks like the tests are passing here, so maybe it's not an issue? I see that numpy 2.2 is being used by the test worker.

Sorry, I mistyped, I meant float and explained myself poorly! model.defaults.gen_encoder_scales calls data.utils.compute_xarray_data_resolution where this fix is made.

@tom-andersson tom-andersson merged commit 191dfd7 into main Jan 15, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants