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

LAPACK: validate input parameters to throw informative errors #53631

Merged
merged 3 commits into from
Mar 12, 2024

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Mar 7, 2024

This PR validates the input parameters to the Julia LAPACK wrappers, so that the error messages are more informative.
On nightly

julia> using LinearAlgebra

julia> LAPACK.geev!('X', 'X', rand(2,2))
 ** On entry to DGEEV  parameter number  1 had an illegal value
ERROR: ArgumentError: invalid argument #1 to LAPACK call

This PR

julia> using LinearAlgebra

julia> LAPACK.geev!('X', 'X', rand(2,2))
ERROR: ArgumentError: argument #1: jobvl must be one of ('N', 'V'), but 'X' was passed

Secondly, moved certain allocations (e.g. in geevx) below the validation checks, so that these only happen for valid parameter values.

Thirdly, added require_one_based_indexing checks to functions where these were missing.

@jishnub jishnub added the linear algebra Linear algebra label Mar 7, 2024
@jishnub jishnub added the error messages Better, more actionable error messages label Mar 7, 2024
@jishnub jishnub requested a review from ViralBShah March 7, 2024 10:07
Copy link
Member

@ViralBShah ViralBShah left a comment

Choose a reason for hiding this comment

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

Looking great!

@ViralBShah ViralBShah added the backport 1.11 Change should be backported to release-1.11 label Mar 7, 2024
@KristofferC KristofferC mentioned this pull request Mar 7, 2024
60 tasks
@jebej
Copy link
Contributor

jebej commented Mar 7, 2024

Some of these functions do a lot of checks, could there be a performance impact on fast calls?

@jishnub
Copy link
Contributor Author

jishnub commented Mar 7, 2024

There are some O(N^2) infinite/NaN checks that may be relegated to post-call error reporting. I suspect this will help with performance. I am planning a separate PR for better post-call error messages. The checks in this PR are O(1), so they shouldn't impact performance.

@oscardssmith
Copy link
Member

IMO this is a bad idea. We don't want to encourage people to use the LAPACK interfaces directly because the character interface is bad, and the function names are bad. Adding extra checks for using a low level library directly that should instead be called via wrappers that ensure correct construction is IMO not the right way to go here.

@ViralBShah
Copy link
Member

ViralBShah commented Mar 7, 2024

@oscardssmith I don't agree. These are not exported interfaces - and hence it is clear to people what they are. While we are under no obligation to make them nice, people do use these, and they should work nicely. If someone writes a new Julian LAPACK interface at some point, we can always point people there.

@ViralBShah
Copy link
Member

There are some O(N^2) infinite/NaN checks that may be relegated to post-call error reporting. I suspect this will help with performance. I am planning a separate PR for better post-call error messages. The checks in this PR are O(1), so they shouldn't impact performance.

I was going to mention that we could do a lot better on post-call error messages. Looking forward!

@jishnub jishnub merged commit dcd1fb2 into master Mar 12, 2024
7 of 9 checks passed
@jishnub jishnub deleted the jishnub/lapackchkvalid branch March 12, 2024 02:04
KristofferC pushed a commit that referenced this pull request Mar 15, 2024
This PR validates the input parameters to the Julia LAPACK wrappers, so
that the error messages are more informative.
On nightly
```julia
julia> using LinearAlgebra

julia> LAPACK.geev!('X', 'X', rand(2,2))
 ** On entry to DGEEV  parameter number  1 had an illegal value
ERROR: ArgumentError: invalid argument #1 to LAPACK call
```
This PR
```julia
julia> using LinearAlgebra

julia> LAPACK.geev!('X', 'X', rand(2,2))
ERROR: ArgumentError: argument #1: jobvl must be one of ('N', 'V'), but 'X' was passed
```

Secondly, moved certain allocations (e.g. in `geevx`) below the
validation checks, so that these only happen for valid parameter values.

Thirdly, added `require_one_based_indexing` checks to functions where
these were missing.

(cherry picked from commit dcd1fb2)
KristofferC added a commit that referenced this pull request Mar 17, 2024
Backported PRs:
- [x] #39071 <!-- Add a lazy `logrange` function and `LogRange` type -->
- [x] #51802 <!-- Allow AnnotatedStrings in log messages -->
- [x] #53369 <!-- Orthogonalize re-indexing for FastSubArrays -->
- [x] #48050 <!-- improve `--heap-size-hint` arg handling -->
- [x] #53482 <!-- add IR encoding for EnterNode -->
- [x] #53499 <!-- Avoid compiler warning about redefining jl_globalref_t
-->
- [x] #53507 <!-- update staled `Core.Compiler.Effects` documentation
-->
- [x] #53408 <!-- task splitting: change additive accumulation to
multiplicative -->
- [x] #53523 <!-- add back an alias for `check_top_bit` -->
- [x] #53377 <!-- add _readdirx for returning more object info gathered
during dir scan -->
- [x] #53525 <!-- fix InteractiveUtils call in Base.runtests on failure
-->
- [x] #53540 <!-- use more efficient `_readdirx` for tab completion -->
- [x] #53545 <!-- use `_readdirx` for `walkdir` -->
- [x] #53551 <!-- revert "Add @create_log_macro for making custom styled
logging macros (#52196)" -->
- [x] #53554 <!-- Always return a value in 1-d circshift! of
abstractarray.jl -->
- [x] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [x] #53571 <!-- Update Documenter to v1.3 for inventory writing -->
- [x] #53403 <!-- Move parallel precompilation to Base -->
- [x] #53589 <!-- add back `unsafe_convert` to pointer for arrays -->
- [x] #53596 <!-- build: remove extra .a file -->
- [x] #53606 <!-- fix error path in `precompilepkgs` -->
- [x] #53004 <!-- Unexport with, at_with, and ScopedValue from Base -->
- [x] #53629 <!-- typo fix in scoped values docs -->
- [x] #53630 <!-- sroa: Fix incorrect scope counting -->
- [x] #53598 <!-- Use Base parallel precompilation to build stdlibs -->
- [x] #53649 <!-- precompilepkgs: package in boths deps and weakdeps are
in fact only weak -->
- [x] #53671 <!-- Fix bootstrap Base precompile in cross compile
configuration -->
- [x] #52125 <!-- Load Pkg if not already to reinstate missing package
add prompt -->
- [x] #53602 <!-- Handle zero on arrays of unions of number types and
missings -->
- [x] #53516 <!-- permit NamedTuple{<:Any, Union{}} to be created -->
- [x] #53643 <!-- Bump CSL to 1.1.1 to fix libgomp bug -->
- [x] #53679 <!-- move precompile workload back from Base -->
- [x] #53663 <!-- add isassigned methods for reinterpretarray -->
- [x] #53662 <!-- [REPL] fix incorrectly cleared line after completions
accepted -->
- [x] #53611 <!-- Linalg: matprod_dest for Diagonal and adjvec -->
- [x] #53659 <!-- fix #52025, re-allow all implicit pointer casts in
cconvert for Array -->
- [x] #53631 <!-- LAPACK: validate input parameters to throw informative
errors -->
- [x] #53628 <!-- Make some improvements to the Scoped Values
documentation. -->
- [x] #53655 <!-- Change tbaa of ptr_phi to tbaa_value  -->
- [x] #53391 <!-- Default to the medium code model in x86 linux -->
- [x] #53699 <!-- Move `isexecutable, isreadable, iswritable` to
`filesystem.jl` -->
- [x] #41232 <!-- Fix linear indexing for ReshapedArray if the parent
has offset axes -->
- [x] #53527 <!-- Enable analyzegc checks for try catch and fix found
issues -->
- [x] #52092 
- [x] #53682 <!-- Increase build precompilation -->
- [x] #53720 
- [x] #53553 <!-- typeintersect: fix `UnionAll` unaliasing bug caused by
innervars. -->

Contains multiple commits, manual intervention needed:
- [ ] #53305 <!-- Propagate inbounds in isassigned with CartesianIndex
indices -->

Non-merged PRs with backport label:
- [ ] #53736 <!-- fix literal-pow to return the right type when the base
is -1 -->
- [ ] #53707 <!-- Make ScopedValue public -->
- [ ] #53696 <!-- add invokelatest to on_done callback in bracketed
paste -->
- [ ] #53660 <!-- put Logging back in default sysimage -->
- [ ] #53509 <!-- revert moving "creating packages" from Pkg.jl -->
- [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} -->
- [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51928 <!-- Styled markdown, with a few tweaks -->
- [ ] #51816 <!-- User-themable stacktraces -->
- [ ] #51811 <!-- Make banner size depend on terminal size -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error messages Better, more actionable error messages linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants