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

additional minor updates (skimage 0.20) #455

Merged
merged 15 commits into from Nov 30, 2022

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Nov 23, 2022

a few additional late additions from skimage 0.20

  • better warnings/errors related to data_range for structural_similarity
  • fix bug where order of the returned elements from hessian_matrix was reversed
  • fix stacklevel of warning in lab2xyz

and two minor updates to previously merged cucim 2022.12.00 MRs

  • allow a device scalar for the data_range argument for structural_similarity
  • provide dedicated 2D and 3D kernels for hessian_matrix_det

Each of these are in separate commits, so can be reveiwed independently

For floating point inputs, the user must specify data_range.
For integer dtype other than uint8 warn on automatic setting of data_range.
disallow order='xy' with ndim>2 for hessian_matrix

after the changes, order behavior is as documented and is consistent with structure_tensor
e.g. a user might pass `data_range=image.ptp()`
fix output dtype in case of approximate=True
increment by 1 due to presence of the channel_as_last_axis decorator
@grlee77 grlee77 added bug Something isn't working improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Nov 23, 2022
@grlee77 grlee77 added this to the v22.12.00 milestone Nov 23, 2022
@grlee77 grlee77 requested a review from a team as a code owner November 23, 2022 17:46
@grlee77 grlee77 added this to PR-Needs review in v22.12.00 Release via automation Nov 23, 2022
@grlee77 grlee77 removed the bug Something isn't working label Nov 23, 2022
@@ -223,7 +228,7 @@ def _hessian_matrix_with_gaussian(image, sigma=1, mode='reflect', cval=0,

# 2.) apply the derivative along another axis as well
axes = range(ndim)
if order == 'rc':
if order == 'xy':
Copy link
Member

@jakirkham jakirkham Nov 23, 2022

Choose a reason for hiding this comment

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

So the previous behavior here was a bug or should this have remained the same?

Edit: See another line like this below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it was a bug.

In practice, the actual order does not matter for internal functions like skimage.filters.frangi, skimage.feature.blob_doh, or skimage.feature.shape_index that calls this function since these rely only on the eigenvalues or determinant of the matrix which are fortunately the same for either order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change here makes things consistent with skimage.feature.structure_tensor which also uses the same logic, but had the ordering correct.

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 23, 2022

updated to fix style checks

Had to add this condition for the scikit-image CPU-based implementation to pass all tests.
Added it here for the GPU as well for consistency
@grlee77
Copy link
Contributor Author

grlee77 commented Nov 29, 2022

The last commit here resolves warnings raised by the NumPy 1.24 pre-release (CuPy reuses the dtype objects from NumPy and so the deprecation warning would appear here as well). This was fixed along with other warnings not currently relevant to cuCIM in scikit-image/scikit-image#6637

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 29, 2022

The change in 5a8e44c seems advisable. I didn't run into NaNs on the GPU when testing this funcdtion, but when porting the same code to Cython I did still see them in scikit-image. The argument to the sqrt functions should not ever be negative as the eigenvalues of a real, symmetric matrix are always real-valued. In practice, due to floating point errors, tiny negative values might occur in practice and clipping to 0 seems warranted.

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!

v22.12.00 Release automation moved this from PR-Needs review to PR-Reviewer approved Nov 30, 2022
@grlee77
Copy link
Contributor Author

grlee77 commented Nov 30, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 35f0792 into rapidsai:branch-22.12 Nov 30, 2022
v22.12.00 Release automation moved this from PR-Reviewer approved to Done Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants