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

MAINT: spatial: use cython_lapack in spatial/_qhull.pyx #20337

Merged
merged 2 commits into from Mar 27, 2024

Conversation

ev-br
Copy link
Member

@ev-br ev-br commented Mar 26, 2024

Use linalg.cython_lapack in _qhull.pyx instead of manual LAPACK wrappers (the qhull usage likely predates cython_lapack).

Just a small drive-by cleanup while looking at #20271

@ev-br ev-br added scipy.spatial maintenance Items related to regular maintenance tasks labels Mar 26, 2024
@github-actions github-actions bot added C/C++ Items related to the internal C/C++ code base Cython Issues with the internal Cython code base labels Mar 26, 2024
@tylerjereddy tylerjereddy added this to the 1.14.0 milestone Mar 26, 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.

The diff seems sensible and passed full suite locally with my setup as well.

I did notice I could make this cleanup on this branch now, with full suite still passing locally, any reason not do do that:

--- a/scipy/spatial/meson.build
+++ b/scipy/spatial/meson.build
@@ -40,7 +40,7 @@ py3.extension_module('_qhull',
     'qhull_src/src'
   ],
   link_args: version_link_args,
-  dependencies: [lapack, np_dep],
+  dependencies: [np_dep],
   install: true,
   subdir: 'scipy/spatial'
 )

@ev-br
Copy link
Member Author

ev-br commented Mar 27, 2024

Indeed. Added your patch to the PR.

@tylerjereddy tylerjereddy merged commit 25a256b into scipy:main Mar 27, 2024
29 checks passed
@tylerjereddy
Copy link
Contributor

Thanks Evgeni

@h-vetinari
Copy link
Member

Do you think this would help with #20271, even if it's just about easier debugging? If so, should we still backport this to 1.13?

@ev-br
Copy link
Member Author

ev-br commented Apr 1, 2024

That's clutching at straws basically: if we're in luck, the issue was in the manual lapack wrappers, as unlikely as it is. The alternative is rather bleak, it'd likely mean the issue is in vendored qhull code. A stack trace would be really helpful.

@tylerjereddy tylerjereddy added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Apr 1, 2024
@h-vetinari
Copy link
Member

A stack trace would be really helpful.

Yeah, I know. I'll try to look into it when I'm back at my laptop in about a week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C/C++ Items related to the internal C/C++ code base Cython Issues with the internal Cython code base maintenance Items related to regular maintenance tasks scipy.spatial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants