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

CR Hamiltonian experiment without cr_gate #794

Merged

Conversation

nkanazawa1989
Copy link
Collaborator

Summary

Bug fix for CrossResonanceHamiltonian experiment instantiation without cr gate.

Details and comments

self._cr_gate should be assigned before calling super init method since it calls set backend where we check the cr gate.

https://qiskit.slack.com/archives/C02CFNAKURE/p1651458517025279

@nkanazawa1989 nkanazawa1989 added backport stable potential The issue or PR might be minimal and/or import enough to backport to stable Changelog: Bugfix Include in the "Fixed" section of the changelog labels May 2, 2022
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! I wonder if the different cases (with and without backend/gate) should be tested more systematically.

@nkanazawa1989
Copy link
Collaborator Author

Thanks Will, actually I found another bug with extra test 12faf53

self._dt, self._granularity, and self._cr_channel are initialized in the constructor but after the super init method call is accomplished. Since it internally populates these values when it's called with backend through self._set_backend method, these value SHOULD be initialized before the super init method call. Otherwise, populated values are overridden. It's really critical bug.

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.

I wonder though if you should more systematically have four tests, with/without backend and with/without cr_gate that all make sure circuits works without error. I guess you cover all cases except no backend with cr_gate currently.

@nkanazawa1989
Copy link
Collaborator Author

Thanks,

no backend with cr_gate currently

This is tested in test_instance_with_backend_without_cr_gate. When we set the backend later, this is identical to setting them together.

@nkanazawa1989 nkanazawa1989 merged commit 2119d72 into qiskit-community:main May 9, 2022
@nkanazawa1989 nkanazawa1989 deleted the fix/cr_ham_constructor branch May 9, 2022 00:39
chriseclectic pushed a commit to chriseclectic/qiskit-experiments that referenced this pull request May 18, 2022
* Bug fix; instantiate CR Ham expr without cr_gate

* fix another bug with extra unittest
chriseclectic pushed a commit to chriseclectic/qiskit-experiments that referenced this pull request May 18, 2022
* Bug fix; instantiate CR Ham expr without cr_gate

* fix another bug with extra unittest
paco-ri pushed a commit to paco-ri/qiskit-experiments that referenced this pull request Jul 11, 2022
* Bug fix; instantiate CR Ham expr without cr_gate

* fix another bug with extra unittest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport stable potential The issue or PR might be minimal and/or import enough to backport to stable Changelog: Bugfix Include in the "Fixed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants