Skip to content

Conversation

@gabuzi
Copy link
Contributor

@gabuzi gabuzi commented Nov 18, 2024

Hi,

I thought I'd make the package available on conda-forge so that it can be used itself as a dependency for building other conda packages. See here for the corresponding PR conda-forge/staged-recipes#28200

During building the conda package, I emphasized installability of the package in the conda-forge ecosystem for the tests after the conda package is built, which means I deliberately did not pin the test requirements, which then defaults to installing with numpy 2.x. This revealed the test incompatibility with numpy 2.0 that this PR addresses so that tests can pass with numpy 2.x. It was a simple deprecation of the np.product alias for np.prod, see https://numpy.org/doc/stable/numpy_2_0_migration_guide.html#main-namespace.

An open issue is that by default, the install also with pytorch 2.x, which currently breaks the Toeplitz notebook, but there's already a corresponding issue here #98

Please let me know if you would like to be added as a recipe maintainer for conda-forge, @mmuckley

np.product was removed in numpy 2.0, and only np.prod remains.
This commit allows tests to pass under numpy 2.0
@mmuckley mmuckley self-requested a review November 22, 2024 14:16
Copy link
Owner

@mmuckley mmuckley left a comment

Choose a reason for hiding this comment

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

Thanks so much @gabuzi!

You can add me as a maintainer for the conda-forge recipe so that if needed I can apply a patch, but as you can probably tell I'm not spending as much time on torchkbnufft these days so can't guarantee how responsive I'll be :).

With this change maybe we should update the test versions to something a bit more modern for an end-of-year release.

@mmuckley mmuckley merged commit 7e23776 into mmuckley:main Nov 22, 2024
4 checks passed
@gabuzi
Copy link
Contributor Author

gabuzi commented Nov 25, 2024

Hi @mmuckley

I have added you as a maintainer. It's clear that this is all "best-effort", so no worries about responsivenss – as you say, I think it's just essential to have multiple people in the loop with push access!

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