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.

Sign up for GitHub

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

Jump to bottom

Standardize on rng over seed and fix miscellaneous deprecation warnings #621

Merged
merged 8 commits into from
Nov 4, 2023
Merged

Standardize on rng over seed and fix miscellaneous deprecation warnings #621

merged 8 commits into from
Nov 4, 2023

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Oct 28, 2023

This PR fixes the following warnings observed when running the test suite:

src/cucim/core/operations/morphology/tests/test_distance_transform.py: 90 warnings
  /pyenv/versions/3.9.18/lib/python3.9/site-packages/cucim/core/operations/morphology/_distance_transform.py:160: DeprecationWarning: Conversion of an array with ndim > 0 to a scalar is deprecated, and will error in future. Ensure you extract a single element from your array before performing this operation. (Deprecated NumPy 1.25.)
    scalar_sampling = float(unique_sampling)

 src/cucim/skimage/feature/tests/test_corner.py::test_custom_eigvals_kernels_vs_linalg_eigvalsh[float64-shape2]
  /__w/cucim/cucim/python/cucim/src/cucim/skimage/feature/tests/test_corner.py:345: FutureWarning: use_gaussian_derivatives currently defaults to False, but will change to True in a future version. Please specify this argument explicitly to maintain the current behavior
    H = hessian_matrix(img)

 src/cucim/skimage/morphology/tests/test_binary.py: 148 warnings
  /__w/cucim/cucim/python/cucim/src/cucim/skimage/morphology/tests/test_binary.py:68: FutureWarning: `seed` is a deprecated argument name for `binary_blobs`. It will be removed in version 0.23. Please use `rng` instead.
    img = cp.asarray(data.binary_blobs(32, n_dim=ndim, seed=1))

@grlee77 grlee77 added bug Something isn't working non-breaking Introduces a non-breaking change labels Oct 28, 2023
@grlee77 grlee77 added this to the v23.12.00 milestone Oct 28, 2023
@grlee77 grlee77 self-assigned this Oct 28, 2023
@grlee77 grlee77 requested a review from a team as a code owner October 28, 2023 19:11
gigony
gigony approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@gigony gigony left a comment

Choose a reason for hiding this comment

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

Thanks @grlee77 !
Looks good to me!

jakirkham
jakirkham reviewed Oct 31, 2023
View reviewed changes
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Greg! 🙏

Had a couple questions below

python/cucim/src/cucim/core/operations/morphology/_distance_transform.py Outdated Show resolved Hide resolved
python/cucim/src/cucim/skimage/feature/tests/test_corner.py
@@ -342,7 +342,7 @@ def _reference_eigvals_computation(S_elems):
def test_custom_eigvals_kernels_vs_linalg_eigvalsh(shape, dtype):
rng = cp.random.default_rng(seed=5)
img = rng.integers(0, 256, shape)
H = hessian_matrix(img)
H = hessian_matrix(img, use_gaussian_derivatives=False)
Copy link
Member

Choose a reason for hiding this comment

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

Will we need to change the hessian_matrix's use use_gaussian_derivatives default value at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. It is not marked for change yet in upstream scikit-image.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can write up an issue to track

python/cucim/src/cucim/skimage/morphology/tests/test_binary.py Show resolved Hide resolved
@grlee77
fix NumPy warning on casting array with ndim > 0 to scalar
fde34f0
@grlee77
avoid FutureWarning in corner detection test case by explicitly setti…
4b6fa47 
…ng use_gaussian_derivatives=False
@grlee77
avoid FutureWarning about deprecated seed argument
e72804c
@grlee77
update API to reflect that 'rng' was chosen over 'seed' in upstream s…
4f16f7a 
…cikit-image

missed this late change from seed->rng in previous scikit-image 0.21 API updates in cuCIM
@grlee77
avoid unecessary indexing for float conversion
45fa113 
use cp.unique/cp.atleast_1d directly instead of passing cupy.ndarray to numpy
@grlee77 grlee77 force-pushed the grelee/fix-deprecation-warnings branch from 857a7ba to 45fa113 Compare November 1, 2023 14:25
@grlee77 grlee77 changed the title Fix miscellaneous deprecation warnings Standardize on rng over seed and fix miscellaneous deprecation warnings Nov 2, 2023
jakirkham
jakirkham reviewed Nov 3, 2023
View reviewed changes
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Greg! 🙏

Had a few comments below

python/cucim/src/cucim/skimage/data/_binary_blobs.py Show resolved Hide resolved
python/cucim/src/cucim/core/operations/morphology/_distance_transform.py Show resolved Hide resolved
python/cucim/src/cucim/skimage/_shared/utils.py Outdated Show resolved Hide resolved
python/cucim/src/cucim/skimage/data/_binary_blobs.py Show resolved Hide resolved
python/cucim/src/cucim/skimage/data/_binary_blobs.py Show resolved Hide resolved
python/cucim/src/cucim/skimage/feature/tests/test_corner.py
@@ -342,7 +342,7 @@ def _reference_eigvals_computation(S_elems):
def test_custom_eigvals_kernels_vs_linalg_eigvalsh(shape, dtype):
rng = cp.random.default_rng(seed=5)
img = rng.integers(0, 256, shape)
H = hessian_matrix(img)
H = hessian_matrix(img, use_gaussian_derivatives=False)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can write up an issue to track

@grlee77 @jakirkham
Apply suggestions from code review
9dc6665 
Co-authored-by: jakirkham <jakirkham@gmail.com>
jakirkham
jakirkham approved these changes Nov 3, 2023
View reviewed changes
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Greg! 🙏

@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit f26381a into rapidsai:branch-23.12 Nov 4, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakirkham jakirkham jakirkham approved these changes

@gigony gigony gigony approved these changes

Assignees

@grlee77 grlee77

Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
cucim
Status: Done
Milestone
v23.12.00
Development

Successfully merging this pull request may close these issues.
3 participants
@grlee77 @jakirkham @gigony