Skip to content

ENH:MAINT:linalg det in Cython and with nDarray support #18225

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 17, 2023
Merged

ENH:MAINT:linalg det in Cython and with nDarray support #18225

merged 7 commits into from
Apr 17, 2023

Conversation

ilayn
Copy link
Member

@ilayn ilayn commented Apr 1, 2023 •
edited
Loading

Reference issue

Related to #18208

What does this implement/fix?

  • Removed the dependency to custom Fortran code in /linalg/src/det.f
  • Added some more tests
  • Touched the documentation for cosmetics
  • Performance is improved (depends highly on LAPACK lib)
  • Now supports arbitrary stacked arrays; la.det(np.ones([4, 3, 5, 5])) returns a (4, 3) array.
  • Also supports all numeric dtypes (unlike NumPy) float16 is upcasted, complex256 is downcasted
  • Single precision arrays, if they survive LU step, don't overflow in single precision(ex. np.linalg.det(np.diag([1.e38, 1e38]).astype(np.float32)) is still finite in float but gives np.inf)
  • Handles all kinds of empty arrays
  • Quick returns for (1, 1) and (..., 1, 1) shaped arrays
  • Makes at most a single copy of the input to match LAPACK requirements

@ilayn ilayn added enhancement A new feature or improvement scipy.linalg Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org maintenance Items related to regular maintenance tasks labels Apr 1, 2023
@ilayn ilayn requested a review from larsoner as a code owner April 1, 2023 01:22
@ilayn
Copy link
Member Author

ilayn commented Apr 1, 2023

The stats.TestDunnett failures are not related to the PR.

@lesteve lesteve mentioned this pull request Apr 5, 2023
Merged
1 task
@ilayn ilayn added this to the 1.11.0 milestone Apr 6, 2023
@ilayn
Copy link
Member Author

ilayn commented Apr 6, 2023

Any takers for reviewing?

@mdhaber
Copy link
Contributor

mdhaber commented Apr 7, 2023

@ilayn I can review the interface and testing, but not the implementation.

@ilayn
Copy link
Member Author

ilayn commented Apr 7, 2023

That's also very valuable anyways 😅

mdhaber
mdhaber reviewed Apr 9, 2023
View reviewed changes
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.

Can't comment on the implementation, but the tests look pretty comprehensive (unless you want to check out what happens when there are NaNs?). Just a few questions and suggestions.

Also, it might be good to document how this differs from np.linalg.det. (Or maybe I'm just curious.)

mdhaber
mdhaber approved these changes Apr 9, 2023
View reviewed changes
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.

With some final tweaks, LGTM from the perspective of documentation and tests.
@ev-br would you be willing to review implementation and merge?

@j-bowhay
Copy link
Member

j-bowhay commented Apr 9, 2023

Can det.f be tidied up and removed as a result of this?

@ilayn
Copy link
Member Author

ilayn commented Apr 9, 2023 •
edited
Loading

Yes, all flinalg stuff would be possible to remove once this one and lu (PR) is in. I'm waiting this first then I'll submit LU.

@ilayn
Copy link
Member Author

ilayn commented Apr 14, 2023

Should we click the button on this one? I'm hoping to make it to 1.11 release with this one and prospective LU PR to get rid of _flinalg stuff to reduce Fortranisms.

@mdhaber mdhaber requested a review from ev-br April 14, 2023 16:37
@ilayn ilayn mentioned this pull request Apr 17, 2023
Merged
@ilayn
Copy link
Member Author

ilayn commented Apr 17, 2023

@tylerjereddy already proposed the 1.11 release schedule so good pressure is on me about the LU stuff. Hence if you don't mind I'd like to click the button in about 20 hours 😅

@ev-br
Copy link
Member

ev-br commented Apr 17, 2023

FWIW I won't be able to look at it in the next 20 hours, and there's no need for me to block the progress here :-).

@ilayn
Copy link
Member Author

ilayn commented Apr 17, 2023

Thanks @ev-br anyways, and @mdhaber for review and all ! Let me get back to the actual troubling part of LU 😃

@ilayn ilayn merged commit 156050b into scipy:main Apr 17, 2023
@ilayn ilayn deleted the det_no_fortran branch April 17, 2023 16:57
@ilayn ilayn mentioned this pull request May 9, 2023
Closed
@ilayn ilayn mentioned this pull request May 27, 2023
Open
37 tasks
@mdhaber mdhaber mentioned this pull request Jun 23, 2023
Closed
@mdhaber
Copy link
Contributor

mdhaber commented May 31, 2024

@ilayn I was wondering if you'd have a chance to add ndarray support to scipy.linalg.solve_triangular? I've wanted to add ndarray support to the multivariate normal distribution, and I can use NumPy for eigh, cholesky, and a few others, but I'd need to loop for solve_triangular since NumPy doesn't have it. LMK if you'd like me to create an issue (or if there already is one for the n-d upgrades). Thanks for considering it!

@ilayn
Copy link
Member Author

ilayn commented May 31, 2024

Yes @mdhaber the idea is at some point to enhance linalg functions to batch array support. I don't see any blockers for that happening. It is just a matter of allocating time.

The longer story is that linalg.solve_triangular should have been inside linalg.solve anyways. In other words solve should be the one-stop shop for all solve things whether by auto-discovery of triangle structure, or through the assume_a by skipping the checks. In the meantime, if singularity is not a problem you can loop through trsm from linalg.blas.

None of these remarks are helping you right now obviously, so let me finish id_dist then fix #20813 and then I can add the ndarray support for solve_triangular as I don't see any fundamental issues with it.A separate issue is definitely useful as a reminder indeed.

@mdhaber mdhaber mentioned this pull request Aug 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdhaber mdhaber mdhaber approved these changes

@larsoner larsoner Awaiting requested review from larsoner larsoner is a code owner

@ev-br ev-br Awaiting requested review from ev-br

Assignees
No one assigned
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org enhancement A new feature or improvement maintenance Items related to regular maintenance tasks scipy.linalg
Projects
None yet
Milestone
1.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ilayn @mdhaber @j-bowhay @ev-br