Skip to content

Moved functions from ndimage submodules to ndimage namespace#325

Merged
GenevieveBuckley merged 2 commits intodask:mainfrom
m-albert:move_functions_to_ndimage_namespace
Aug 1, 2023
Merged

Moved functions from ndimage submodules to ndimage namespace#325
GenevieveBuckley merged 2 commits intodask:mainfrom
m-albert:move_functions_to_ndimage_namespace

Conversation

@m-albert
Copy link
Copy Markdown
Collaborator

This PR moves functions from the cupyx.scipy.ndimage.filters namespace to cupyx.scipy.ndimage, fixing #324.

Secondly it changes a reference to scipy.ndimage.fourier into scipy.ndimage in test functions, anticipating the removal of scipy.ndimage.fourier in scipy version 2.

@GenevieveBuckley
Copy link
Copy Markdown
Collaborator

This is great, thank you @m-albert

I think we need a note somewhere in the docs that says what the minimum supported version of cupy is.

@GenevieveBuckley
Copy link
Copy Markdown
Collaborator

I think we need a note somewhere in the docs that says what the minimum supported version of cupy is.

I've searched through the repo, and it looks like we mostly put that information in the release notes (HISTORY.rst file). What is the new minimum version of cupy?

And before we merge and make a new release, can I get you to check whether the pytest importerskip lines need to be updated with a different minimum version of cupy? There are 8 lines like this across the codebase
cupy = pytest.importorskip("cupy", minversion="...

@jakirkham
Copy link
Copy Markdown
Member

Thanks you both! 🙏

Do these tests run on CI cover those function changes? Or is there something more we need?

@m-albert
Copy link
Copy Markdown
Collaborator Author

Thanks @GenevieveBuckley

And before we merge and make a new release, can I get you to check whether the pytest importerskip lines need to be updated with a different minimum version of cupy? There are 8 lines like this across the codebase
cupy = pytest.importorskip("cupy", minversion="...

Good point. @rgommers mentioned that all filters were always also (and mainly) available in the ndimage namespace. Probably this is also true for the rest of ndimage?

I checked when cupyx functions first became available and updated the minversion lines. If I'm not mistaken, some versions were off, e.g. most filters were only added to cupyx in version 9.0.0 (instead of 7.7.0) and affine_transform was already available in 5.0.0 (instead of 6.0.0).

I've searched through the repo, and it looks like we mostly put that information in the release notes (HISTORY.rst file). What is the new minimum version of cupy?

Would this be the minimum version that provides at least some functionality or the minimum version that provides all dask-image related cupy functionality?

Do these tests run on CI cover those function changes? Or is there something more we need?

@jakirkham The function changes should be covered! The reason for why the removal of cupyx.scipy.ndimage.filters in cupy>=11.0 hadn't been picked up must be because cupy=10.6 is installed on the GPU CI, as shown by the console output.

@m-albert
Copy link
Copy Markdown
Collaborator Author

Probably it'd be useful to have these fixes for #324 included in the upcoming release. Do you guys think this PR is mergeable?

@GenevieveBuckley GenevieveBuckley merged commit fba6645 into dask:main Aug 1, 2023
@GenevieveBuckley
Copy link
Copy Markdown
Collaborator

Probably it'd be useful to have these fixes for #324 included in the upcoming release. Do you guys think this PR is mergeable?

Sure, I'm on board with that.
It looks like we should be fairly safe recommending people use cupy version 9.0.0 and above in the next set of release notes. Thanks for following up on this @m-albert

@jakirkham
Copy link
Copy Markdown
Member

It looks like we should be fairly safe recommending people use cupy version 9.0.0 and above in the next set of release notes.

Filed issue ( #328 )

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.

3 participants