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.value_indices: deal with unfixed types #20644

Merged
merged 1 commit into from May 15, 2024

Conversation

tylerjereddy
Copy link
Contributor

[skip circle] [skip cirrus]

@tylerjereddy tylerjereddy added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.ndimage backport-candidate This fix should be ported by a maintainer to previous SciPy versions. labels May 5, 2024
@tylerjereddy tylerjereddy added this to the 1.14.0 milestone May 5, 2024
@github-actions github-actions bot added the C/C++ Items related to the internal C/C++ code base label May 5, 2024
@tylerjereddy
Copy link
Contributor Author

tylerjereddy commented May 5, 2024

ah right, I'll need preprocessor guards or something similar, that's fun

@tylerjereddy
Copy link
Contributor Author

tylerjereddy commented May 5, 2024

That's a little closer, but 32-bit Linux failure is real. I could probably skip that case and argue we're still doing better than main, but I should probably do a better/more complete fix I suppose.

@lucascolley lucascolley changed the title BUG: value_indices unfixed types BUG: ndimage.value_indices: deal with unfixed types May 5, 2024
* Fixes scipygh-19423

* Add a few more `case` statements to account for
the (i.e., Windows) data types that don't have a fixed
width, and add a regression test.

[skip circle] [skip cirrus]
@tylerjereddy
Copy link
Contributor Author

Alright, this seems to be portable now, and CI is passing. There may be a more elegant solution, though the patch isn't too big anyway.

@neilflood
Copy link
Contributor

Thanks @tylerjereddy

I wrote the original switch statements, and had no idea that these generic names had such different behaviour on Windows. Thanks for rescuing that, much appreciated. :-)

@tylerjereddy
Copy link
Contributor Author

@grlee77 are you happy for this to be merged? I don't usually add shims to ndimage, but looks like original author of these code blocks is happy.

@tylerjereddy
Copy link
Contributor Author

I'm going to take the approval of the original author of the code as a substitute/"next best thing" for core dev approval and self-merge this ahead of the 1.13.1 release. Ping me if you're not happy about that. The CI is passing, it has a regression test, and the patch is small.

@tylerjereddy tylerjereddy merged commit 2dbb27e into scipy:main May 15, 2024
29 checks passed
@tylerjereddy tylerjereddy deleted the treddy_issue_19423 branch May 15, 2024 17:31
@lucascolley lucascolley modified the milestones: 1.14.0, 1.13.1 May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-candidate This fix should be ported by a maintainer to previous SciPy versions. C/C++ Items related to the internal C/C++ code base defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.ndimage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: scipy.ndimage.value_indices returns empty dict for intc/uintc dtype on Windows
3 participants