-
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
Catalyst supports dynamic_one_shot #5766
Conversation
…ements can accept results with a broadcast dimension (for jitting).
Hello. You may have forgotten to update the changelog!
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5766 +/- ##
==========================================
- Coverage 99.66% 99.66% -0.01%
==========================================
Files 415 415
Lines 39703 39489 -214
==========================================
- Hits 39571 39356 -215
- Misses 132 133 +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.
Looks good, although unfortunately between all the squeezing, stacking, list packing and unpacking, and without already knowing the expected data format in different places I found it quite hard to review those kinds of changes and will mostly have to trust you about them 😅
Co-authored-by: David Ittah <dime10@users.noreply.github.com>
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.
Looks good, although unfortunately between all the squeezing, stacking, list packing and unpacking, and without already knowing the expected data format in different places I found it quite hard to review those kinds of changes and will mostly have to trust you about them 😅
Right, sorry about that. I think once all MCM-related PRs go in, this will be a good candidate for refactoring.
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.
Looks great so far! The failing CI seems problematic, and I left a few comments about the changes to the transform as well.
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
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.
Looks great, just one stylistic optional suggestion. Other than that, happy to approve once code coverage is passing.
Code coverage will not pass until the CI can pull a Catalyst version supporting the one-shot mode. So should we force merge and come back to it once the Catalyst support is out? |
I'd prefer to avoid force merging. You can add |
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.
Left a comment about using # pragma: no cover
to silence the coverage errors. An alternative would be to mock qml.compiler.active
to always return True
and use the mocked function in unit tests.
All other changes look good to me and I'm happy to approve once this coversation is resolved.
Thanks @mudit2812 . I'll go with |
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
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.
Looks great, just a couple of minor things:
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.
Happy to approve 💯
Before submitting
Please complete the following checklist when submitting a PR:
All new features must include a unit test.
If you've fixed a bug or added code that should be tested, add a test to the
test directory!
All new functions and code must be clearly commented and documented.
If you do make documentation changes, make sure that the docs build and
render correctly by running
make docs
.Ensure that the test suite passes, by running
make test
.Add a new entry to the
doc/releases/changelog-dev.md
file, summarizing thechange, and including a link back to the PR.
The PennyLane source code conforms to
PEP8 standards.
We check all of our code against Pylint.
To lint modified files, simply
pip install pylint
, and thenrun
pylint pennylane/path/to/file.py
.When all the above are checked, delete everything above the dashed
line and fill in the pull request template.
Context:
Catalyst does not currently support hardware-like execution for dynamic circuits.
Description of the Change:
Modify
parse_native_mid_circuit_measurements
to handle measurements with a broadcast dimension.Add special handling of terminal measurements of MCMs.
Benefits:
Possible Drawbacks:
Related GitHub Issues:
[sc-60756]