Fix white_tophat/black_tophat crash with structure-only argument (#9756)#9768
Fix white_tophat/black_tophat crash with structure-only argument (#9756)#9768nicholasreichert wants to merge 4 commits intocupy:mainfrom
Conversation
Fixes cupy#9756: white_tophat and black_tophat raised ValueError when called with only the structure parameter (no size or footprint). This regression was introduced in cupy#8339 when axes support was added. - Pass structure to _check_nd_args so it does not raise when weights and sizes are both None but structure is provided - Match SciPy's boolean XOR behavior in white_tophat/black_tophat - Add regression tests for structure-only calls
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Thanks @nicholasreichert! A few comments, mainly wondering if a simple re-order isn't sufficient (and comments about tests).
| [True, True, True], | ||
| [True, True, True], | ||
| ]) | ||
| return scp.ndimage.white_tophat(x, structure=structure) |
There was a problem hiding this comment.
Can you please make this a single parametrized test (also may want to just use eye and ones or so for the test array.
| return self._filter(xp, scp, x) | ||
|
|
||
|
|
||
| @testing.with_requires('scipy') |
There was a problem hiding this comment.
| @testing.with_requires('scipy') |
accidental duplicate
| input, ftprnt, mode, origen, 'footprint', sizes=size, axes=axes, | ||
| raise_on_zero_size_weight=True) | ||
| raise_on_zero_size_weight=structure is None, | ||
| structure=structure) |
There was a problem hiding this comment.
It's not clear to me that this is a very clean approach. Can you check whether we can't just re-order things so that _check_size_footprint_structure is called first here?
| tmp = grey_erosion(tmp, size, footprint, structure, output, **kwargs) | ||
| if input.dtype == numpy.bool_ and tmp.dtype == numpy.bool_: | ||
| if tmp is None: | ||
| tmp = output |
There was a problem hiding this comment.
Looks good, but do we have a test that hits this branch?
Will take a look over the weekend and get back to you asap. Sorry about duplicate, trying out nvim but definitely a learning curve. |
It's rather hidden, but |
3 participants
Fixes #9756: white_tophat and black_tophat raised ValueError: must specify either weights array or sizes when called with only the structure parameter (no size or footprint). This regression was introduced in #8339 when axes support was added.
Changes
_filters_core.py: Added structure parameter to _check_nd_args. When weights=None and sizes=None but structure is provided, use the structure shape for origen validation instead of raising ValueError.
_filters.py: Pass structure to _check_nd_args in _min_or_max_filter, and only raise on zero-size weight when no structure is provided.
_morphology.py: Match SciPy's behavior in white_tophat/black_tophat using bitwise_xor instead of subtract when both input and output are boolean arrays.
test_morphology.py: Added TestWhiteBlackTopHatStructureOnly regression tests.
Testing:
python -m pytest tests/cupyx_tests/scipy_tests/ndimage_tests/test_morphology.py
4947 passed, 330 skipped