-
Notifications
You must be signed in to change notification settings - Fork 603
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
Add test workflow for legacy opmath #5435
Conversation
Hello. You may have forgotten to update the changelog!
|
[sc-59553] |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5435 +/- ##
==========================================
- Coverage 99.67% 99.66% -0.01%
==========================================
Files 406 406
Lines 37881 37590 -291
==========================================
- Hits 37757 37465 -292
- Misses 124 125 +1 ☔ View full report in Codecov by Sentry. |
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.
This looks really good, thanks @lillian542 !
My main concern is the change to qml.ops.Hamiltonian
. One of the main reasons we went through the trouble of creating LinearCombination
and dispatching qml.Hamiltonian
to either class was to be able to leave ops.Hamiltonian
untouched to continue to guarantee the performance in the lightning paper.
If there is a way to not have this change I would suggest to go that route.
CC @maliasadi for the discussion on performance numbers. |
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 again @lillian542!
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.
Really good job @lillian542 thanks a lot!
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 @lillian542!
Then we will add it to the test matrix (separate PR to modify the plugin test matrix repo).
Do we have a separate story to track this?
Currently, the tests only run with the new opmath, so won't know if we introduced a bug that breaks the legacy opmath behaviour while it is still in its deprecation cycle.
This PR creates a kwarg,
disable-opmath
, that can be passed when running the tests, and a corresponding workflow that runs tests with that kwarg set to True. This can also be used locally, i.e.python -m pytest tests/ --disable-opmath=True
Right now, it runs with CI on the PR, but once the tests are all passing, the line triggering that will be removed and it will only run every 3-4 days, in the middle of the night. Then we will add it to the test matrix (separate PR to modify the plugin test matrix repo).
The changes to .yml files and to the
conftest
files are all about allowing us to run these additional tests. A few modifications to tests were made to allow them to pass with both legacy opmath and new opmath.There is one test currently marked as xfail for new opmath that I would call a bug - I opened an issue here: #5512