Reinstate qmax argument to dlasq3 and slasq3 for parity with reference LAPACK and cython_lapack#24
Merged
Conversation
Owner
|
Wow that is weird that I missed it. That means no test it covering it, which I come to realize slowly about LAPACK in general unfortunately. I'm planning to make a hail mary attempt to make testing suite a bit better and possibly push it to upstream but that Fortran testing mechanism is making me itch. I'll see what I can do. But you seem to be a much better analyzer than intellisense 😅 anyways. Clicked the button. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reference LAPACK's
dlasq3/slasq3and SciPy's cython_lapack takeqmaxas an in/out parameter, but we don't have it in the signature here (but we did have a stale param doc line though). Not sure why it was removed! :) I checked the commit history and found 7b823f6 and c75b3d1. The former commit had qmax by value in dlasq3, and then the latter removed it. The static analysis was right, though. However, the analyser shouldn't find this to be an issue (I tested withclang --analyze -Xclang -analyzer-output=text, at least).See here:
This PR restores the argument, the reversal-branch update of it, and adds it back through
dlasq2/slasq2, the Fortran-ABI shim, and the public headers.Also xref pyodide/pyodide-recipes#619, where everything is coming together now! I am using this PR as a patch there and will drop it when this is merged. This came up in one of the
scipy.linalgtest crashes in local testing. With this patch, the function signature mismatch is fixed.