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

Verify values of parametrized rzz gates #2021

Merged
merged 45 commits into from
Dec 3, 2024
Merged

Verify values of parametrized rzz gates #2021

merged 45 commits into from
Dec 3, 2024

Conversation

yaelbh
Copy link
Collaborator

@yaelbh yaelbh commented Nov 7, 2024

Summary

Fixes #2000.

This is only a draft. I forgot to write the tests before the code. So the current push contains the logic, but the tests will be added shortly.

Details and comments

@SamFerracin I have a question about the noise learner. I see that NoiseLearner._run calls validate_isa_circuits, like BasePrimitiveV2. I added in BasePrimitivesV2 a call to a new function, validate_rzz_pubs. But I'm not familiar enough with the noise learner to figure if a similar call should be added there too. I tried to add, but NoiseLearner._run receives circuits, unlike BasePrimitiveV2, which receives pubs. Should I make _run receive pubs and add the call to the new function?

@yaelbh yaelbh marked this pull request as draft November 7, 2024 12:41
@yaelbh
Copy link
Collaborator Author

yaelbh commented Nov 11, 2024

Following #2035, we should also take into account parameter expressions.

@yaelbh
Copy link
Collaborator Author

yaelbh commented Nov 12, 2024

A couple of comments:

  1. The check for fixed angles will take place in both is_isa_circuit and is_rzz_pub. In is_isa_circuit - because not everyone has a full pub and can call is_rzz_pub - see the noise learner. In is_rzz_pub - because this way the function makes more sense and is more complete, this will be less confusing for developers, who may naturally expect fixed angles to be covered.
  2. Parameter expressions are much more complicated, and will be computationally more expensive. An expression can consist of multiple parameters. We will have to evaluate each expression with all the parameter values for its parameters. At least for now this PR continues to skip parameter expressions.

Copy link
Member

@t-imamichi t-imamichi 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 since I'm not an official approver, you need to ask someone else to review it too.

@yaelbh yaelbh requested a review from kt474 November 18, 2024 11:33
Copy link
Member

@kt474 kt474 left a comment

Choose a reason for hiding this comment

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

LGTM, can you add a release note?

@yaelbh
Copy link
Collaborator Author

yaelbh commented Nov 24, 2024

@kt474 @wshanks It's ready for another review. Note that I added release notes.

Copy link
Collaborator

@wshanks wshanks left a comment

Choose a reason for hiding this comment

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

This looks good to me except for a minor formatting change on the release note. I put in two other minor suggestions.

release-notes/unreleased/2021.feat.rst Outdated Show resolved Hide resolved
qiskit_ibm_runtime/utils/validations.py Outdated Show resolved Hide resolved
test/unit/test_sampler.py Outdated Show resolved Hide resolved
@yaelbh yaelbh requested a review from wshanks November 26, 2024 08:19
Copy link
Collaborator

@wshanks wshanks left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@kt474 kt474 added the Changelog: New Feature Include in the Added section of the changelog label Dec 2, 2024
@yaelbh yaelbh merged commit 681aeb9 into Qiskit:main Dec 3, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the Added section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider handling rzz validation for parametrized angles
5 participants