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

Fix of null column error in geqrf! and gerqf! #42844

Merged
merged 6 commits into from
Nov 11, 2021
Merged

Fix of null column error in geqrf! and gerqf! #42844

merged 6 commits into from
Nov 11, 2021

Conversation

andreasvarga
Copy link
Contributor

Addresses #42762. This is my first PR. I hope it is correct. No tests have been performed.

Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

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

Thanks the PR. It would be good if you could also add a test for the case that used to error.

Moving BlasInt outside max will also allow you to use it just once but that isn't a big deal.

@andreasvarga
Copy link
Contributor Author

I did not add a test because I don't know the expected correct output.

@dkarrasch dkarrasch added the linear algebra Linear algebra label Oct 29, 2021
@andreasvarga
Copy link
Contributor Author

I think
lwork = max(BlasInt(1),BlasInt(real(work[1])))
is in general more efficient than
lwork = BlasInt(max(1,real(work[1])))
Although both involve two conversions, the former involves two conversions to integer and max of two integers,
while the latter involves a conversion to real, max of two reals and a conversion to integer.

So I propose to leave it as it is now in the former form.

Just a question regarding this, my first, PR: What I have to do to finish, since two checks failed, but apparently they have nothing to do with the changes performed in this PR.

@N5N3
Copy link
Member

N5N3 commented Oct 31, 2021

lwork = max(BlasInt(1),BlasInt(real(work[1]))) should be better as there's no need for max to handle NaN inputs. (Although this won't be the bottleneck)
As for CI, tester_freebsd64 is known broken for a long time; tester_linuxaarch64 seems to be related with OpenBLAS itself. So I think you can ignore them here.

As for the test, I think someting like:

a = rand(2,0), zeros(0) # just copy from the related issue
@test LinearAlgebra.LAPACK.geqrf!(a...) === a 
@test LinearAlgebra.LAPACK.gerqf!(a...) === a # this won't pass even with this fix on my PC.

would be enough.

@andreasvarga
Copy link
Contributor Author

andreasvarga commented Nov 1, 2021

The correction for gerqf! was more tricky. Here the issue was that for null column dimension, the required workspace must be at least M, the first dimension of the matrix. So, calling the LAPACK routine with the returned "optimal" value of LWORK = 1, still leads to error -7. So, I had to modify the workspace adjustement to
lwork = max(m, 1, Int(real(work[1])))
Probably
lwork = max(m, Int(real(work[1])))
would also do the job.

Many thanks N5N3 for the help and pointing out this issue.

stdlib/LinearAlgebra/test/lapack.jl Outdated Show resolved Hide resolved
stdlib/LinearAlgebra/src/lapack.jl Outdated Show resolved Hide resolved
@andreasnoack
Copy link
Member

andreasnoack commented Nov 1, 2021

The correction for gerqf! was more tricky.

I'm pretty sure that should be considered a bug in LAPACK.

@andreasvarga
Copy link
Contributor Author

I opened an issue for LAPACK at #633, where I described the problem more in detail.

@ViralBShah
Copy link
Member

@andreasvarga Could you rebase this to master?

@andreasvarga
Copy link
Contributor Author

This being my first PR, I am afraid I need some guidance what I have exactly to do. Sorry for this!

@ViralBShah
Copy link
Member

ViralBShah commented Nov 10, 2021

@andreasvarga Welcome and thank you for making the PR!

On the master branch do git pull to update to the latest julia source code. Then switch to your branch and do git rebase master. This will apply your changes on top of the latest julia source (which will basically pull in other fixes and most likely make the failing tests pass).

@andreasvarga
Copy link
Contributor Author

I followed your hints. Here is what I got:

$ git pull
remote: Enumerating objects: 1497, done.
remote: Counting objects: 100% (986/986), done.
remote: Compressing objects: 100% (177/177), done.
remote: Total 1497 (delta 833), reused 926 (delta 805), pack-reused 511
Receiving objects: 100% (1497/1497), 1.63 MiB | 3.64 MiB/s, done.
Resolving deltas: 100% (1029/1029), completed with 267 local objects.
From https://github.com/JuliaLang/julia
 + 93e0a8b065...6d9fc140cf aa/libunwind-1.5.0    -> origin/aa/libunwind-1.5.0  (forced update)
 * [new branch]            avi/inlinedoc         -> origin/avi/inlinedoc
   f52c6368e2..cca9bf3530  backports-release-1.6 -> origin/backports-release-1.6
   21a2784994..8597a4b6d3  backports-release-1.7 -> origin/backports-release-1.7
 + 3f15b33c75...fb61f37daa dk/tridiagsolve       -> origin/dk/tridiagsolve  (forced update)
 * [new branch]            jb/bidi               -> origin/jb/bidi
 * [new branch]            jb/faster-resolve-scopes -> origin/jb/faster-resolve-scopes
   2ab960e113..23fcd8437e  jb/stripmeta          -> origin/jb/stripmeta
 * [new branch]            jn/fastlocks          -> origin/jn/fastlocks
 * [new branch]            ksh/chartests         -> origin/ksh/chartests
   c054dbc6d4..7ac710cbf7  master                -> origin/master
   69455be4cd..e990ca91db  oscardssmith-AbstractRNG-docs -> origin/oscardssmith-AbstractRNG-docs
 + e791da70c2...c8f5d11b54 oscardssmith-exhaustive-Float160-tests -> origin/oscardssmith-exhaustive-Float160-tests  (forced update)
 * [new branch]            sds/extended_slurp    -> origin/sds/extended_slurp
 * [new branch]            sds/rest_svec         -> origin/sds/rest_svec
 * [new branch]            sv-keep-hidden        -> origin/sv-keep-hidden
 + 5fedc98405...669ad59ed7 teh/active_project_watchers -> origin/teh/active_project_watchers  (forced update)
 + 30d09a877e...4130432b6c vs/rm-openlibm        -> origin/vs/rm-openlibm  (forced update)
There is no tracking information for the current branch.
Please specify which branch you want to merge with.
See git-pull(1) for details.

    git pull <remote> <branch>

If you wish to set tracking information for this branch you can do so with:

    git branch --set-upstream-to=<remote>/<branch> av/lapack


$ git rebase master
Current branch av/lapack is up to date.

@ViralBShah
Copy link
Member

Right - also do git push -f now to force push your changes to your branch.

@ViralBShah
Copy link
Member

Actually I am not sure this has fully rebased it to master. I think the steps are:

git checkout master
git pull
git checkout av/lapack
git rebase master
git push -f

I wonder if things work differently on forked repos.

@andreasvarga
Copy link
Contributor Author

I performed the last step as:

$ git remote -v
avarga  git@github.com:andreasvarga/julia.git (fetch)
avarga  git@github.com:andreasvarga/julia.git (push)
origin  https://github.com/JuliaLang/julia.git (fetch)
origin  https://github.com/JuliaLang/julia.git (push)

$ git push -f avarga av/lapack
Enumerating objects: 45, done.
Counting objects: 100% (45/45), done.
Delta compression using up to 16 threads
Compressing objects: 100% (38/38), done.
Writing objects: 100% (38/38), 3.31 KiB | 1.65 MiB/s, done.
Total 38 (delta 29), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (29/29), completed with 6 local objects.
To github.com:andreasvarga/julia.git
 + cdc018b409...d8f2aa7845 av/lapack -> av/lapack (forced update)

I hope I did not something wrong.

@ViralBShah
Copy link
Member

It looks like it happened correctly. Thank you.

@andreasnoack Are we ok to merge this if CI looks good? I doubt the upstream will do anything anytime soon.

@andreasnoack andreasnoack merged commit c25b704 into JuliaLang:master Nov 11, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
* Fix of null column error in geqrf! and gerqf!

* Added tests

* Trailing blanks removed

* Correct issue number

* Adjusted lwork to max(m, 1, Int(real(work[1])))

* Fine tuning
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
* Fix of null column error in geqrf! and gerqf!

* Added tests

* Trailing blanks removed

* Correct issue number

* Adjusted lwork to max(m, 1, Int(real(work[1])))

* Fine tuning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants