-
Notifications
You must be signed in to change notification settings - Fork 56
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
Bugfixes and efficiency improvements for basis conversion, inner products, and computing Frobenius distance #432
Conversation
…not needed and which were only correct for real inputs). Replace the np.trace(np.dot(...)) pattern for computing the trace inner product with np.vdot(...). Use of np.vdot has a secondary affect of conjugating the first argument when dealing with complex inputs, which resolves a limitation of basis conversion discussed in an email thread.
Haven't done a full review yet, but I think I have the most familiarity with the interpygate code so I will put that on my todo list. The interpygate tests are much closer to integration tests than true unit tests, so it is possible the nominal value was wrong, but needs more investigation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work, @rileyjmurray! I've left a few minor questions for you as part of my review, but by and large these changes all look good, and should bring some performance gains to boot!
As for the failing unit tests you've encountered, I pulled the latest version of this branch and tested this locally on my machine and was unable to reproduce this behavior. That would likely point to this either being a non-deterministic error, or else something configuration dependent in which case we'd need to track this down a bit more. One bit of low-hanging fruit we can try to eliminate is whether this is somehow macOS specific, so I'll manually launch the mac runners on github to see if we can identify anything there. |
Update: I ran the unit tests in question on the runners, and with the exception of the MacOS runner for python 3.8 (which failed because of perennial problems with compiling CVXOPT that typically fix themselves after a while) these all passed. So whatever is going on it seems to be localized to the particular combination of packages/configuration on your machine, @rileyjmurray. Concerns about potential testing issues was the main reason why I didn't mark this as approved when doing my review, so I'm going to switch that designation now that I'm seeing these passing. The other question I had I am still interested in, but these aren't blocking concerns/questions in any way. |
…_optools.py::GateOpsTester
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few more comments and questions related to your most recent changes.
@sserita, I've gotten Corey's sign-off. Can you approve and merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @coreyostrove and @rileyjmurray for the great work :)
Changes:
np.trace(np.dot(...
when computing the trace inner product or Frobenius norm. #430._hack_sqrtm
function. It also hasOld text
Some tests are failing. I'm not sure why. Maybe they're related to the conjugation that's applied when using
vdot
in the basis conversion? If so, I would guess that the failing tests are comparing to a nominal value which actually was not correct to begin with. Thoughts, @coreyostrove, @sserita, @enielse?-- No one was able to reproduce these errors (including me, later on!).