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

Remove int-type bug on Windows in skimage.measure.label #72

Merged
merged 1 commit into from
Jul 28, 2021
Merged

Remove int-type bug on Windows in skimage.measure.label #72

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 2 additions & 22 deletions python/cucim/src/cucim/skimage/measure/_label_kernels.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,10 @@ def _label(x, structure, y, greyscale_mode=False):
y_shape = cupy.array(y.shape, dtype=numpy.int32)
count = cupy.zeros(2, dtype=numpy.int32)
_kernel_init()(x, y)
try:
int_t = int_types[y.dtype.char]
except KeyError:
raise ValueError("y must have int32, uint16, uint32 or uint64 dtype")
Copy link
Member

Choose a reason for hiding this comment

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

Am curious why this exception is no longer needed. What happens if we don't raise this?

Copy link
Contributor Author

@grlee77 grlee77 Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It is a private function that is only called here with an input that is hard-coded to np.int32.

cucim/python/cucim/src/cucim/skimage/measure/_label.py

Lines 120 to 121 in 1aa5802

labels = cp.empty(input.shape, order="C", dtype=cp.int32)
num = _label(input, structure, labels, greyscale_mode=True)

The kernels in this file originate from CuPy, but have minor modifications here to match scipy.skimage.measure.label behavior rather than scipy.ndimage.label. The int checking stuff is not in upstream CuPy, I think I had originally added these so we could switch to int64 if needed, but I'm not sure there is a real use case for > 2 billion labels?

Copy link
Member

Choose a reason for hiding this comment

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

So what happens if a user does pass in an int64 array?

Should add this most likely would happen just because a user requested a dtype of int, which would be handled as int64. Not so much because they need more labels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what happens if a user does pass in an int64 array?

You will get the following compilation error:

E   /tmp/tmp0iyn3i4v/3ac4bcb15a8927c9397d795e29cb7ccd_2.cubin.cu(43): error: no instance of overloaded function "atomicCAS" matches the argument list
E               argument types are: (long long *, Y, Y)

I can add a check in _label that np.dtype(input) == np.float32 with a message like the old one if you like. That can only happen if users are calling that (private) _label function directly, though!

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok. In that case would it make sense to raise in label? Also do we want to update this todo?

cucim/python/cucim/src/cucim/skimage/measure/_label.py

Lines 16 to 17 in 00cd5d6

# TODO: currently uses int32 for the labels. should add int64 option as well
def label(input, background=None, return_num=False, connectivity=None):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no restriction on the type of input here which is the only parameter passed on to _label that comes from the user. The structure and labels array inputs to _label are generated internally, not provided by the user.

The only way the user can encounter the error cited here #72 (comment) would be if they were to import the private _label function from cucim.skimage.measure._label_kernels.

if int_t != "int":
raise NotImplementedError(
"Currently only 32-bit integer case is implemented"
)
Comment on lines -29 to -31
Copy link
Member

Choose a reason for hiding this comment

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

Same question here

if greyscale_mode:
_kernel_connect(True, int_t)(
x, y_shape, dirs, ndirs, x.ndim, y, size=y.size
)
_kernel_connect(True)(x, y_shape, dirs, ndirs, x.ndim, y, size=y.size)
else:
_kernel_connect(False, int_t)(
y_shape, dirs, ndirs, x.ndim, y, size=y.size
)
_kernel_connect(False)(y_shape, dirs, ndirs, x.ndim, y, size=y.size)
_kernel_count()(y, count, size=y.size)
maxlabel = int(count[0]) # synchronize
labels = cupy.empty(maxlabel, dtype=numpy.int32)
Expand Down Expand Up @@ -184,11 +172,3 @@ def _kernel_finalize():
""",
"cucim_nd_label_finalize",
)


int_types = {
"i": "int",
"H": "unsigned short",
"I": "unsigned int",
"L": "unsigned long long",
}