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

dynamic_one_shot supports broadcasting; broadcast_expand supports shot vectors #5473

Merged
merged 28 commits into from
Apr 17, 2024

Conversation

mudit2812
Copy link
Contributor

@mudit2812 mudit2812 commented Apr 4, 2024

Context:
Native mid-circuit measurements with default.qubit are not compatible with parameter broadcasting. Due to the complexity of a "native" implementation, I decided to use broadcast_expand, but realized that it does not work with shot vectors.

Description of the Change:
This PR does two things:

  • Update dynamic_one_shot transform to use broadcast_expand and process batched results correctly.
  • Update broadcast_expand to support shot vectors.
  • Raise error when postselecting with broadcasting and returning samples. This change was made to both dynamic_one_shot and defer_measurements because both transforms use broadcast_expand for broadcasting, although defer_measurements only uses broadcast_expand with postselection.

Note that broadcasting with qml.sample and postselection will still not work due to ragged dimensions. If reviewers are okay with it, I would like to merge this and leave that improvement as technical debt. cc @trbromley @isaacdevlugt.
Edit about note: Talked offline, decided to raise a more informative error if a user requests postselection with broadcasting when returning samples.

Benefits:
Both transforms are more capable.

Possible Drawbacks:
Because of the stacking and squeezing happening in the post-processing function of broadcast_expand, counts dictionaries get wrapped inside 0-D numpy arrays, which makes indexing into the dict impossible. To access the dictionary and its contents, one has to use array.item() to extract the single item inside the array.

Related GitHub Issues:
#5443

@mudit2812
Copy link
Contributor Author

[sc-60581] [sc-59941]

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.66%. Comparing base (a47d9bc) to head (7569291).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5473      +/-   ##
==========================================
- Coverage   99.67%   99.66%   -0.01%     
==========================================
  Files         406      406              
  Lines       37881    37623     -258     
==========================================
- Hits        37758    37498     -260     
- Misses        123      125       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mudit2812 mudit2812 marked this pull request as ready for review April 9, 2024 20:25
Copy link
Contributor

@dwierichs dwierichs left a comment

Choose a reason for hiding this comment

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

Just some small comments/requests, and a question about those wrapped dicts.
Very nice extension of the recursion logic both in code and tests 💯 💪
And great testing @mudit2812 🎉

pennylane/transforms/broadcast_expand.py Outdated Show resolved Hide resolved
pennylane/transforms/broadcast_expand.py Show resolved Hide resolved
pennylane/transforms/broadcast_expand.py Show resolved Hide resolved
pennylane/transforms/broadcast_expand.py Outdated Show resolved Hide resolved
pennylane/transforms/dynamic_one_shot.py Show resolved Hide resolved
pennylane/transforms/broadcast_expand.py Outdated Show resolved Hide resolved
Copy link
Contributor

@vincentmr vincentmr left a comment

Choose a reason for hiding this comment

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

validate_measurements needs a small fix, but otherwise looks good.

pennylane/transforms/broadcast_expand.py Outdated Show resolved Hide resolved
Copy link
Contributor

@vincentmr vincentmr left a comment

Choose a reason for hiding this comment

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

LGMT, good job @mudit2812 !

Copy link
Contributor

@dwierichs dwierichs left a comment

Choose a reason for hiding this comment

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

Thanks for the additional fix of Conditional @mudit2812 ! 💯

Is there a test that would fail without that fix?

Maybe you could also mention the fix of Conditional in the changelog? And shouldn't there be some unit tests of Conditional affected or bind_new_parameters affected somehow? 🙃

pennylane/transforms/dynamic_one_shot.py Show resolved Hide resolved
pennylane/ops/op_math/condition.py Show resolved Hide resolved
Copy link
Contributor

@dwierichs dwierichs left a comment

Choose a reason for hiding this comment

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

LGTM!

doc/releases/changelog-dev.md Show resolved Hide resolved
pennylane/ops/op_math/condition.py Show resolved Hide resolved
@mudit2812 mudit2812 enabled auto-merge (squash) April 16, 2024 10:09
@mudit2812 mudit2812 disabled auto-merge April 16, 2024 14:21
@mudit2812 mudit2812 added the do not merge ⚠️ Do not merge the pull request until this label is removed label Apr 16, 2024
@mudit2812 mudit2812 enabled auto-merge (squash) April 17, 2024 15:35
@mudit2812 mudit2812 merged commit 2b47a00 into master Apr 17, 2024
38 checks passed
@mudit2812 mudit2812 deleted the dos-broadcast branch April 17, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge ⚠️ Do not merge the pull request until this label is removed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Finite-shot mid-circuit measurement support is not compatible with broadcasting
6 participants