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

[query] ndarray inverse using LAPACK #9093

Merged
merged 3 commits into from Jul 17, 2020
Merged

Conversation

akotlar
Copy link
Contributor

@akotlar akotlar commented Jul 17, 2020

to flesh out our linear algebra, and in service of regenie. eventually, this, qr, and other linalg functions should be moved to hl.linalg I think, to match scipy, numpy, dask, etc

we should also decide whether to break out LU factorization. Numpy, scipy, dask do this, so that's my preference for a followup pr, because people who are looking for a fleshed out linalg experience may want this.

also, we should have less than double precision support, but this matches QR.

@akotlar akotlar changed the title [query] ndarray inverse [query] ndarray inverse using LAPACK Jul 17, 2020
@@ -2244,6 +2244,14 @@ class IRSuite extends HailSuite {
assertEvalsTo(makeNDArrayRef(matMulCube, IndexedSeq(0, 0, 0)), 30.0)
}

@Test def testNDArrayInv() {
implicit val execStrats: Set[ExecStrategy] = ExecStrategy.compileOnly
Copy link
Contributor Author

@akotlar akotlar Jul 17, 2020

Choose a reason for hiding this comment

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

It's not clear to me if this is an oversight, but most of our NDArray tests are not actually tested, because execStrats are Set(); the values specified in these assertEvalsTo are arbitrary. See TestNDArrayMatMul as a worrisome example of this. This was done during move away from C++ and not fixed after John re-implemented the work in Scala. I will PR a fix unless there are other plans around this.

Copy link
Contributor

Choose a reason for hiding this comment

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

oof, good catch! That's consistent with my memory of the history here. Will happily (and a bit nervously...) review the PR to reenable most tests.

These nodes are also tested pretty well in Python, so this isn't as horrifying as it may appear at first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I verified that matmul for instance works perfectly enabled.

Copy link
Contributor

@tpoterba tpoterba left a comment

Choose a reason for hiding this comment

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

Awesome! Nice work, Alex!

@akotlar
Copy link
Contributor Author

akotlar commented Jul 17, 2020

it was fun, thanks Tim!

@danking danking merged commit 1039d16 into hail-is:master Jul 17, 2020
@johnc1231
Copy link
Contributor

Very cool!

@akotlar akotlar deleted the ndarray-inv branch July 17, 2020 21:04
Dania-Abuhijleh pushed a commit to Dania-Abuhijleh/hail that referenced this pull request Jul 23, 2020
* implementation for doubles

* cleanup

* flake8 format
annamiraotoole pushed a commit to annamiraotoole/hail that referenced this pull request Aug 3, 2020
* implementation for doubles

* cleanup

* flake8 format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants