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

Fix adjoint validation with global measurements #5761

Merged
merged 6 commits into from
May 31, 2024
Merged

Conversation

dwierichs
Copy link
Contributor

@dwierichs dwierichs commented May 29, 2024

Context:
Validation of the adjoint method did not take device wires into account, leading to the linked issue.

Description of the Change:
Include validate_device_wires in _supports_adjoint in DefaultQubit.

Benefits:

Possible Drawbacks:

Related GitHub Issues:
fixes #5760

[sc-64278]

Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

Potentially we could add a device_wires keyword argument to adjoint_state_measurements, and then pass in device_wires or adjoint_state_measurements to the StateMP. And if both are None, maybe we just default to one qubit?

While this would work for the validation, would it actually work for the execution part?

@dwierichs
Copy link
Contributor Author

Is the current approach not a good idea? Note that the execution is not affected by the bug, it's just the validation, because it calls _add_adjoint_transforms(prog) before using the device wire validation to update the global measurement types.

Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

Ignore earlier comment. This seems to be a perfectly straightforward fix that gets the job done :)

@dwierichs dwierichs added the review-ready 👌 PRs which are ready for review by someone from the core team. label May 29, 2024
Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni left a comment

Choose a reason for hiding this comment

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

Thanks @dwierichs , LGTM!

@dwierichs dwierichs enabled auto-merge (squash) May 30, 2024 13:44
@dwierichs dwierichs added merge-ready ✔️ All tests pass and the PR is ready to be merged. and removed review-ready 👌 PRs which are ready for review by someone from the core team. labels May 30, 2024
Copy link

codecov bot commented May 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.66%. Comparing base (f5b805b) to head (ea30d1c).
Report is 270 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5761      +/-   ##
==========================================
- Coverage   99.67%   99.66%   -0.02%     
==========================================
  Files         416      413       -3     
  Lines       38686    38813     +127     
==========================================
+ Hits        38562    38684     +122     
- Misses        124      129       +5     

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

@dwierichs dwierichs merged commit 544b1de into master May 31, 2024
40 checks passed
@dwierichs dwierichs deleted the adjoint-validation branch May 31, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-ready ✔️ All tests pass and the PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Adjoint method does not respect global wires return types
3 participants