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

Use alternative CS decomposition when it is missing from LAPACK #485

Merged
merged 11 commits into from
Dec 2, 2019

Conversation

jmbr
Copy link
Contributor

@jmbr jmbr commented Nov 19, 2019

No description provided.

@jmbr jmbr added enhancement New feature or request work-in-progress labels Nov 19, 2019
@jmbr jmbr requested a review from a team as a code owner November 19, 2019 00:51
@notmgsk
Copy link
Member

notmgsk commented Nov 19, 2019

; Symbol "HERMITIAN-EIG" not found in the MAGICL package.

@jmbr
Copy link
Contributor Author

jmbr commented Nov 19, 2019

; Symbol "HERMITIAN-EIG" not found in the MAGICL package.

That's only available in MAGICL's master or, preferably, in quil-lang/magicl#65

@jmbr jmbr changed the title Implement CS decomposition in case the underlying LAPACK implementation does not have it Implement CS decomposition in case the underlying LAPACK library does not have it Nov 19, 2019
@notmgsk
Copy link
Member

notmgsk commented Nov 19, 2019

; Symbol "HERMITIAN-EIG" not found in the MAGICL package.

That's only available in MAGICL's master or, preferably, in rigetti/magicl#65

Ah. Then maybe we ought to make a MAGICL release so that I will be picked up here.

@jmbr
Copy link
Contributor Author

jmbr commented Nov 19, 2019

Ah. Then maybe we ought to make a MAGICL release so that I will be picked up here.

Yeah, that's reasonable.

@appleby
Copy link
Contributor

appleby commented Nov 20, 2019

Would it make sense for the csd-compat implementation to live in the magicl package, and have magicl fallback to it if zuncsd isn't available? Or maybe just provide it separately? I only skimmed the diff, but IIUC it looks like csd.lisp only depends on m* from cl-quil, which might also be reasonably moved into magicl.

@jmbr
Copy link
Contributor Author

jmbr commented Nov 20, 2019

The reason this code presently lives in cl-quil is because it implements a specific case of the CSD (currently the one used in Quantum Shannon compilation). If it ever becomes a general-purpose CSD implementation, then I agree we might consider moving it to magicl.

@jmbr
Copy link
Contributor Author

jmbr commented Nov 25, 2019

Passes the test suite. Closes #292.

@jmbr jmbr removed the do not merge label Nov 25, 2019
@jmbr jmbr requested a review from notmgsk November 25, 2019 15:31
@jmbr jmbr changed the title Implement CS decomposition in case the underlying LAPACK library does not have it Use alternative CS decomposition when it is missing from LAPACK Nov 25, 2019
@appleby
Copy link
Contributor

appleby commented Nov 25, 2019

Should we bump the dependency in cl-quil.asd to magicl 0.6.5?

I guess we're probably waiting on the quicklisp release

@jmbr
Copy link
Contributor Author

jmbr commented Nov 25, 2019 via email

src/csd.lisp Show resolved Hide resolved
@appleby
Copy link
Contributor

appleby commented Nov 25, 2019

Passes the test suite. Closes #292.

By "passes the test suite" do you mean you installed a version of LAPACK without ZUNCSD and ran the test suite, or just that the new csd tests you added + rest of the test suite passes when run against an LAPACK that includes ZUNCSD.

@jmbr
Copy link
Contributor Author

jmbr commented Nov 25, 2019

Passes the test suite. Closes #292.

By "passes the test suite" do you mean you installed a version of LAPACK without ZUNCSD and ran the test suite, or just that the new csd tests you added + rest of the test suite passes when run against an LAPACK that includes ZUNCSD.

The former: I used Apple's Accelerate framework (which does not include ZUNCSD) to run the entire test suite (I also double-checked that the new routines were picked up by traceing them).

app/src/entry-point.lisp Outdated Show resolved Hide resolved
src/csd.lisp Outdated Show resolved Hide resolved
src/csd.lisp Show resolved Hide resolved
tests/csd-tests.lisp Outdated Show resolved Hide resolved
src/csd.lisp Outdated Show resolved Hide resolved
@notmgsk notmgsk merged commit 942b107 into master Dec 2, 2019
@notmgsk notmgsk deleted the enhancement/cs-decomposition branch December 2, 2019 19:51
@notmgsk notmgsk mentioned this pull request Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants