Remove int-type bug on Windows in skimage.measure.label #72
Conversation
This was causing a bug on Windows where np.dtype(np.int32).dtype.char == 'l' instead of 'i'
grlee77
added
bug
| try: | ||
| int_t = int_types[y.dtype.char] | ||
| except KeyError: | ||
| raise ValueError("y must have int32, uint16, uint32 or uint64 dtype") |
There was a problem hiding this comment.
Am curious why this exception is no longer needed. What happens if we don't raise this?
There was a problem hiding this comment.
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
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| raise NotImplementedError( | ||
| "Currently only 32-bit integer case is implemented" | ||
| ) |
|
@gpucibot merge |
|
Thanks Greg! 😄 |
A windows user reported an issue with
skimage.measure.labelin the oldercupyimgrepository: mritools/cupyimg#21.It seems the same issue is present here, but should be resolved by this PR. I don't think we can easily add a test case on our current linux-based CI as the issue has to do with the NumPy dtype character used to represent the
numpy.int32dtype. We may want to wait on merging until the fix is confirmed in mritools/cupyimg#21I think I originally introduced this
int_typesdict with the intention of also supporting >32-bit integers, but it was incomplete and still usedint32in the actual implementation. This PR just removes this dict altogether and we can revisit the function if we get a request for int64 support.