Update cucim.skimage API to match scikit-image 0.19

[grlee77]

This PR builds on top of #189 and introduces new deprecations as in scikit-image 0.19.

closes #98

Specifically:
selem -> footprint
grey -> gray
iterations -> num_iter
max_iter -> max_num_iter
min_iter -> min_num_iter

[ajschmidt8]

We'll also need to get the scikit-image version updated in the integration repository below. @grlee77, can you open up a PR for this?

https://github.com/rapidsai/integration/blob/f0848c2c340de9e511c72d6edcc66e2b310f1c9e/conda/recipes/versions.yaml#L147-L148

[grlee77]

@jakirkham , @gigony : I think I now have all the updates from scikit-image 0.19 in this one PR. Aside from the basic updates, I did add four new functions introduced in 0.19 that were possible purely with the CuPy API:

skimage.filters.butterworth
skimage.measure.blur_effect
skimage.metrics.normalized_mutual_information
skimage.transform.resize_local_mean

Aside from the updates to 0.19, I also ran pytest --doctest-modules and fixed any failing doctests. It should be possible to test on CI with scikit-image 0.18 or 0.19 (We mainly use scikit-image to retrieve test data that is the same in both versions).

[jakirkham]

We'll also need to get the scikit-image version updated in the integration repository below. @grlee77, can you open up a PR for this?

https://github.com/rapidsai/integration/blob/f0848c2c340de9e511c72d6edcc66e2b310f1c9e/conda/recipes/versions.yaml#L147-L148

@ajschmidt8, done in PR ( rapidsai/integration#414 ) :)

[grlee77]

tests are close to passing now. It looks like there was one test case in src/cucim/skimage/restoration/tests/test_j_invariant.py that assumed scikit-image 0.19 was installed. I can update that to work for either version.

There were a handful of other failures that look unrelated to the PR. e.g. for the centos7 case I saw

00:42:03 tests/unit/clara/test_image_cache.py:18: in test_get_nocache
00:42:03     from cucim import CuImage
00:42:03 E   ImportError: cannot import name 'CuImage' from 'cucim' (/workspace/python/cucim/src/cucim/__init__.py)

[jakirkham]

There were a handful of other failures that look unrelated to the PR. e.g. for the centos7 case I saw

00:42:03 tests/unit/clara/test_image_cache.py:18: in test_get_nocache
00:42:03     from cucim import CuImage
00:42:03 E   ImportError: cannot import name 'CuImage' from 'cucim' (/workspace/python/cucim/src/cucim/__init__.py)

So I opened PR ( #198 ) to test CI with a trivial blank line addition. However it did not show the same failures. Perhaps there is more still going on here?

[grlee77]

So I opened PR ( #198 ) to test CI with a trivial blank line addition. However it did not show the same failures. Perhaps there is more still going on here?

Although the PR was made against branch-22.02, I must have started this from branch-21.12 (at least, running git describe --abbrev=0 --tags was giving v21.12.00). I rebased and force-pushed the branch and hopefully it will work this time around...

[jakirkham]

Looks like we are still seeing the issue on CI.

Noticed the PR includes some tricks around lazy loading, defining __dir__ in the top-level module, etc. Is it possible something like this is causing issues?

Saw some changes around pytest setup and teardown as well. Wouldn't think that would be related, but that might be another avenue to explore

[jakirkham]

@gigony would you be able to take a look as well? There are some import changes that it would be good to confirm work for you 🙂

[grlee77]

Okay, it passed now. I susupect it was possibly the conftest.py, but did not test with that change in isolation. We do not necessarily need that file here (We use it in one CI case on scikit-image where it is set to fail if any unexpected warnings are encountered).

@gigony, would specifically want to know if you think we should keep the change in c8177c0 that added lazy loading to the top-level cucim (I accidentally commited that in the same commit with the flake8 fix). If not we can revert the change to cucim/__init__.py and just have lazy loading within the cucim.skimage subpackage. Basically the primary benefit is that users only need to import top-level cucim and could then directly call things like cucim.skimage.filters.sobel without having to do a separate import cucim.skimage.filters first. That change was made for scikit-image in 0.19, but we don't necesarilly have to extend it up from cucim.skimage to the top-level cucim module.

[grlee77]

One other option we could potentially do on CI is to run one case with environment variable EAGER_IMPORT=1 defined. That can be used to test with eager importing of everything (i.e. disables lazy loading). That can help rule out potential issues with circular imports.

[gigony]

Looks good to me!

It was good to see how lazy module works :)

Left minor comments.

[gigony]

I wonder why some code is changed to cp.uint8 and some other code is changed to np.uint8.

[grlee77]

I will can change it to cp.uint8 for consistency. In practice it won't matter since cp.uint8 is np.uint8

[grlee77]

updated in 8438d64

[gigony]

@gigony, would specifically want to know if you think we should keep the change in c8177c0 that added lazy loading to the top-level cucim (I accidentally commited that in the same commit with the flake8 fix). If not we can revert the change to cucim/init.py and just have lazy loading within the cucim.skimage subpackage. Basically the primary benefit is that users only need to import top-level cucim and could then directly call things like cucim.skimage.filters.sobel without having to do a separate import cucim.skimage.filters first. That change was made for scikit-image in 0.19, but we don't necesarilly have to extend it up from cucim.skimage to the top-level cucim module.

I think it is okay and having it would make it easier to restruct modules under clara or apply lazy imports in the future.
Thanks for the update!

[ajschmidt8]

Approving ops-codeowner file changes

[jakirkham]

@gpucibot merge