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

Minor fixes for NumPy 2.0 compatibility #746

Merged
merged 2 commits into from
Jul 11, 2024
Merged

Minor fixes for NumPy 2.0 compatibility #746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3236dd8
cast scalar value to array (needed to produce output of correct dtype…
grlee77 Jul 10, 2024
cc787fd
bump tolerance in stain_extraction_pca test
grlee77 Jul 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions python/cucim/src/cucim/skimage/segmentation/_chan_vese.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,7 @@ def chan_vese(
energies = []
phivar = tol + 1

dt = cp.asarray(dt, dtype=float_dtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM to me also.

Just FYI, I'll note that in some places using float(dt) may also make sense to ensure "weak" promotion handling (i.e. float scalars behave like they used to in most cases on both NumPy 1 and 2).
I don't think this needs a fix here!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think here we realized that we were passing a Python float to a CuPy-based custom kernel and relying on whatever default promotion it was doing, which CuPy changed recently. There's more context in 3 under comment: #742 (comment)

So we decided it was better that we determine the type here instead. Hence the change

Hopefully that context is helpful

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, NumPy 2 + CuPy 13.2 interaction might make promotion a bit more tricky even, so I guess the Python float thing may not work there.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah actually in the case where the error was seen, we are using the default value for dt (as it is unspecified)

cucim/python/cucim/src/cucim/skimage/segmentation/tests/test_chan_vese.py

Line 46 in fbca7fa

result = chan_vese(img, mu=0.0, tol=1e-8, extended_output=True)

Looking at the default value for dt, it appears to be 0.5

cucim/python/cucim/src/cucim/skimage/segmentation/_chan_vese.py

Line 282 in fbca7fa

dt=0.5,

So perhaps CuPy has decided to always promote float to float64 before CUDA kernel calls

Greg did the investigation here. So likely knows more about the issue

Copy link
Contributor

Choose a reason for hiding this comment

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

All, good, I don't think it's worth to track this down exactly. Eventually (probably with CuPy 14), this shouldn't be necessary for Python floats at least. But it is definitely the pragmatic thing right now.

while phivar > tol and i < max_num_iter:
# Save old level set values
oldphi = phi
Expand Down
2 changes: 1 addition & 1 deletion python/cucim/tests/unit/core/test_stain_normalizer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ def test_result_value(self, image, expected):
stain_extraction_pca(image)
else:
result = stain_extraction_pca(image)
cp.testing.assert_allclose(result, expected)
cp.testing.assert_allclose(result, expected, rtol=1e-6)


class TestStainNormalizerMacenko:
Expand Down