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

BUG: ndimage: fix origin handling for {minimum, maximum}_filter #20653

Merged
merged 1 commit into from
May 9, 2024

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented May 6, 2024

Reference issue

closes #20652

What does this implement/fix?

fixes incorrect assumption that len(origin) should match ndim. It should have instead been len(axes) with a value of 0 inserted afterwards for any non-filtered axis. This same solution was already previously implemented for rank_filter.

New test cases were added that fail with the error mentioned in #20652 prior to this change (they did not fail previously for rank_filter, median_filter or percentile_filter, but it was easy to add those to the test parameters).

@grlee77 grlee77 added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.ndimage labels May 6, 2024
@grlee77 grlee77 self-assigned this May 6, 2024
@lucascolley lucascolley changed the title fix origin handling for minimum_filter and maximum_filter with axes s… BUG: ndimage: fix origin handling for {minimum, maximum}_filter May 6, 2024
Copy link
Contributor

@mdhaber mdhaber left a comment

Choose a reason for hiding this comment

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

I understand most of this since I reviewed those PRs. I see that the fix is the same as what was already done for rank_filter. The test, which makes sense, does fail in main for the minimum/maximum filters but not the others, as expected. So nothing is jumping out at me - but my eyes aren't very sensitive in this module. Would you like for this to be reviewed by someone else who spends more time with this code (or at lest another set of eyes, since I didn't notice the bug originally)?

@grlee77
Copy link
Contributor Author

grlee77 commented May 9, 2024

Would you like for this to be reviewed by someone else who spends more time with this code (or at lest another set of eyes, since I didn't notice the bug originally)?

Thanks for checking it. I think the test case covers the issue well enough so that additional review is probably not needed.

Apparently this is a pretty uncommon code path as no bug has been raised for it since the 1.11 release

@tylerjereddy tylerjereddy added this to the 1.14.0 milestone May 9, 2024
@tylerjereddy tylerjereddy added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label May 9, 2024
Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

I poked around locally and found the same thing as Matt. Sounds like we're in favor of this and fails before/passes after, so I'll put it in.

I'm sometimes a bit nervous about the exact test comparisons on floating-point type arrays like this. I'll assume that those really are guaranteed to be identical, but if they aren't that's a small follow-up fix.

@tylerjereddy tylerjereddy merged commit 7e69656 into scipy:main May 9, 2024
31 checks passed
@tylerjereddy
Copy link
Contributor

thanks both

@tylerjereddy tylerjereddy modified the milestones: 1.14.0, 1.13.1 May 15, 2024
@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.ndimage
Projects
None yet
3 participants