Skip to content

Update Magicl bindings for Magicl NG - #238

Merged
stylewarning merged 5 commits into
masterfrom
feature/magicl-ng
Feb 28, 2020
Merged

stylewarning merged 5 commits into
masterfrom
feature/magicl-ng

Conversation

@colescott

Copy link
Copy Markdown
Member

This is to update all magicl bindings to work with rigetti/magicl#63.

Benchmarks need to be done prior to merging to make sure there is not a significant performance hit.

@colescott colescott added the / ! \ DO NOT MERGE / ! \ Do not merge! label Feb 3, 2020
@colescott
colescott requested a review from a team as a code owner February 3, 2020 22:15
@colescott colescott self-assigned this Feb 3, 2020

@appleby appleby left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good! Nice to see some examples of the new-and-improved magicl in the wild. Seems like a nice usability & ergonomics improvement.

(reduce #'+ (magicl:matrix-diagonal
(density-matrix (magicl:from-array prepared-state
(list rows cols)
:type '(complex double-float))))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As long as the compiler doesn't complain, maybe the :type here should be qvm:cflonum instead? ditto elsewhere below.

Comment thread dqvm/src/linear-algebra.lisp Outdated
Comment thread src/linear-algebra.lisp Outdated
(make-instance 'quil:simple-gate
:name (concatenate 'string (quil:gate-name gate) "*")
:matrix (magicl:conjugate-entrywise (quil:gate-matrix gate))))
:matrix (magicl:map! #'conjugate (quil:gate-matrix gate))))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does the ! indicate mutation? I don't know if it matters here, but the old conjugate-entrywise did not mutate the input.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@colescott what about this one? ok to map!?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No we very much shall not mutate. You're destroying the environment aka the original gate binding matrix.

Comment thread src/linear-algebra.lisp Outdated
Comment thread tests/basic-noise-qvm-tests.lisp Outdated
Comment thread tests/state-representation-tests.lisp Outdated
@colescott colescott added enhancement New feature or request and removed / ! \ DO NOT MERGE / ! \ Do not merge! labels Feb 27, 2020
@notmgsk

notmgsk commented Feb 28, 2020

Copy link
Copy Markdown
Member

@stylewarning smash the merge button

Comment thread src/linear-algebra.lisp Outdated
(make-instance 'quil:simple-gate
:name (concatenate 'string (quil:gate-name gate) "*")
:matrix (magicl:conjugate-entrywise (quil:gate-matrix gate))))
:matrix (magicl:map! #'conjugate (quil:gate-matrix gate))))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No we very much shall not mutate. You're destroying the environment aka the original gate binding matrix.

Comment thread src/linear-algebra.lisp Outdated
:matrix-function #'(lambda (&rest parameters)
(magicl:conjugate-entrywise
(apply #'quil:gate-matrix gate parameters))))))
(magicl:map! #'conjugate

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no bueno

@stylewarning

Copy link
Copy Markdown
Member

@colescott if u dont stop mutating im going to force us all to write HASKELL instead

@colescott

Copy link
Copy Markdown
Member Author

@colescott if u dont stop mutating im going to force us all to write HASKELL instead

Promise?

@stylewarning
stylewarning merged commit 300a017 into master Feb 28, 2020
@stylewarning
stylewarning deleted the feature/magicl-ng branch February 28, 2020 21:11
@notmgsk notmgsk mentioned this pull request Mar 2, 2020
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