search

icl-utk-edu / lapackpp

LAPACK++ is a C++ wrapper around CPU and GPU LAPACK and LAPACK-like linear algebra libraries, developed as part of the SLATE project.
https://icl.utk.edu/slate/
BSD 3-Clause "New" or "Revised" License
51 stars 14 forks source link

Adding gemqrt function. #33

Closed TeachRaccooon closed 1 year ago

TeachRaccooon commented 1 year ago

Adding a gemqrt() routine, needed in RandLAPACK. Code is initially generated via lapackpp/tools/wrapper_gen.py.

TeachRaccooon commented 1 year ago

Not sure what's causing it to segfault in CI. Will need geqrt in tester, but omitting that shouldn't cause segfaults, it seems.

I was thinking that that is due to the wrong setup of the line in run_tests.py, but seems like something else is the issue. Also, I noticed that there are no tests for gemqr.

TeachRaccooon commented 1 year ago

Just tested gemqrt in RandLAPACK and the routine itself works fine. Looks like something is off with the way I've set up tests.

mgates3 commented 1 year ago

Is there a bug in LAPACK with the workspace query?

Remember, the LAPACK++ wrapper code is written by a Python script reading the LAPACK documentation. It's entirely possible that something goes amiss in that parsing and translation process. The generator is meant to get most of the code, but likely needs tweaks, especially if the documentation doesn't exactly follow LAPACK's (unwritten) style.

TeachRaccooon commented 1 year ago

Is there a bug in LAPACK with the workspace query?

Remember, the LAPACK++ wrapper code is written by a Python script reading the LAPACK documentation. It's entirely possible that something goes amiss in that parsing and translation process. The generator is meant to get most of the code, but likely needs tweaks, especially if the documentation doesn't exactly follow LAPACK's (unwritten) style.

So the generated code did not actually perform the worspace query, I just decided to add it because it was done in gemqr (any my querying strategy was wrong). It looks like gemqrt has a very well-defined workspace size in the first place.

TeachRaccooon commented 1 year ago

Did the manual mapping of transposition types, resolved the last issue.

TeachRaccooon commented 1 year ago

Hi @mgates3 I see that there is apending change request, but I think I addressed all the issues that were there (unless github is not showing me something for whatever reason). So just wanted to check if we can merge this PR.

mgates3 commented 1 year ago

@TeachRaccooon That just means I haven't approved the changes. I'll look at it probably today or tomorrow.

mgates3 commented 1 year ago

I noticed your fork is specific to this feature (TeachRaccooon/lapackpp-add_gemqrt) and uses the master branch in that fork. That works, but it's usually cleaner to create one fork (say, TeachRaccooon/lapackpp) and create a branch for each specific feature (say, branch gemqrt).

mgates3 commented 1 year ago

Can you add an empty commit to trigger the CI actions again? The "re-run jobs" button will re-run with the old master, not with the latest master, which seems odd to me.

git commit --allow-empty -m "Empty-Commit"

I'm going to squash all this into one commit when merging, so the empty commit will disappear.