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.

Sign up for GitHub

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

Jump to bottom

Add twirling options to SamplerV2 #1583

Merged
merged 16 commits into from
Apr 15, 2024
Merged

Add twirling options to SamplerV2 #1583

merged 16 commits into from
Apr 15, 2024

Conversation

ptristan3
Copy link
Collaborator

Summary

Details and comments

Fixes #1557

@ptristan3 ptristan3 added the Changelog: New Feature Include in the Added section of the changelog label Apr 3, 2024
kt474
kt474 approved these changes Apr 3, 2024
View reviewed changes
Copy link
Member

@kt474 kt474 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Minor comment but LGTM - it'd also be good to add/update a test in test_sampler_v2

release-notes/unreleased/1557.feat.rst Outdated Show resolved Hide resolved
ihincks
ihincks reviewed Apr 3, 2024
View reviewed changes
Copy link
Collaborator

@ihincks ihincks left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @ptristan3. It looks like we'll need to reword some of the TwirlingOptions docstrings to make them more generic, as they are specific to the estimator right now.

We should also mention that measurement twirling will only be applied to those measurement registers not involved in conditional logic.

@coveralls
Copy link

coveralls commented Apr 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 8693834567

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on support-twirling at 83.632%
Totals Coverage Status
Change from base Build 8693774513: 83.6%
Covered Lines: 6244
Relevant Lines: 7466
💛 - Coveralls

@ptristan3 ptristan3 marked this pull request as ready for review April 4, 2024 20:12
@ptristan3 ptristan3 requested a review from jyu00 April 4, 2024 20:12
jyu00
jyu00 reviewed Apr 5, 2024
View reviewed changes
Copy link
Collaborator

@jyu00 jyu00 left a comment

Choose a reason for hiding this comment

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

I don't think measurement twirling for sampler is enabled on the server side yet. We should wait for that before merging.

qiskit_ibm_runtime/options/twirling_options.py Outdated Show resolved Hide resolved
@jyu00
Copy link
Collaborator

jyu00 commented Apr 8, 2024

@ptristan3 can we also add twirling to test_program_inputs in TestSamplerOptions? Whether the option gets passed to the server correctly is very important.

jyu00
jyu00 approved these changes Apr 9, 2024
View reviewed changes
Copy link
Collaborator

@jyu00 jyu00 left a comment

Choose a reason for hiding this comment

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

LGTM, but let's not merge until the server side support is ready

@jyu00 jyu00 added the on hold label Apr 9, 2024
@jyu00 jyu00 added this to the 0.23.0 milestone Apr 15, 2024
@jyu00 jyu00 merged commit 7c9979e into Qiskit:main Apr 15, 2024
20 checks passed
@kt474 kt474 removed the on hold label Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihincks ihincks ihincks left review comments

@jyu00 jyu00 jyu00 approved these changes

@kt474 kt474 kt474 approved these changes

Assignees
No one assigned
Labels
Changelog: New Feature Include in the Added section of the changelog
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.

Twirling support in SamplerV2
5 participants
@ptristan3 @coveralls @jyu00 @ihincks @kt474