-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add fake generic and modify tests #10266
Add fake generic and modify tests #10266
Conversation
…d with FakeGeneric
…th instruction_dict in fake_generic.py; added support of grid type of coupling map; using lower number of qubits, passed all tests of test_transpiler except one method ( test_parallel_dispatch_lazy_cal_loading )
…t_fake_generic.py
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
@HuangJunye, I humbly request you to see if this is good enough :) |
Thanks for submitting the PR. I will review it. This PR is part of QAMP Spring 23: qiskit-advocate/qamp-spring-23#26 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the typos
Co-authored-by: atharva-satpute <[email protected]>
Co-authored-by: atharva-satpute <[email protected]>
Co-authored-by: atharva-satpute <[email protected]>
Co-authored-by: atharva-satpute <[email protected]>
@atharva-satpute , done with the changes :) |
@MozammilQ Thanks for the awesome work here! Qiskit core developers really like it. Is it ok for them to take over the PR to finish it up? |
To Qiskit Core developers: Thanks for the kind consideration, this work is not possible without the kind guidance of my mentor @HuangJunye , credit to any such work goes to him ! :) @HuangJunye , around the time we last talked, I did some further work and did some more commits, today I have opened a draft PR for that work. |
@HuangJunye , Yes, it is ok for Qiskit Core to take over the PR to finish it up!! |
Great. Thank you for adding the new work as well. The core team will take a look and work with them too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much @MozammilQ for your contribution. I have added a couple of commits to try to get the PR up to speed, and will now add some comments for reference of the dev. team (no need to act on them) @HuangJunye.
Thanks for the feedback, I have addressed all of the review comments in 54496db, except for the backend name. I think that the candidates are:
I am fine settling for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, a few minor comments / typos.
] | ||
|
||
|
||
class GenericFakeBackend(BackendV2): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the way I see it is, if you don't need a simulator, subclassing this thing won't do anything for you, in which case you really ought to define your own subclass. The name GenericBackendV2
is fine assuming the term "generic" implies "simulator-based", I'm just less convinced that this is a minimal implementor of BackendV2
.
basis_gates if basis_gates is not None else ["cx", "id", "rz", "sx", "x"] | ||
) | ||
for name in ["reset", "delay", "measure"]: | ||
if name not in self._basis_gates: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One trick I found when working in my branch was to use Target.operation_names
to test for instruction presence in constant time. That's backed by a dictionary, I believe, so it's insertion ordered and should side-step the non-determinism issues you were seeing before.
(Not important for this PR IMO given that basis_gates
often small, but perhaps a helpful pattern for the future.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the issue here is that we are constructing a target, so we cannot access its gate map yet... but I see your point, will keep it in mind :)
releasenotes/notes/add-generic-fake-backend-c1434e0c5c413935.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/add-generic-fake-backend-c1434e0c5c413935.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for seeing this through @ElePT and @MozammilQ for your contributions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approval was dismissed after fixing a last-minute merge conflict, so I'm re-approving to get it back in the queue :)
Looks like another merge issue. I'll resolve this and push shortly. |
The problems are likely a minor logical merge conflict with new tests added in #11554, and no inherent problem. |
@Mergifyio backport stable/0.46 |
✅ Backports have been created
|
* Added a FakeGeneric BackendV2 backend, this is just a bare minimum working code * All FakeBackends (like FakeMelbourne, FakeBoeblingen ,etc) is replaced with FakeGeneric * relocated FakeGeneric in fake_provider directory; replaced if-elif with instruction_dict in fake_generic.py; added support of grid type of coupling map; using lower number of qubits, passed all tests of test_transpiler except one method ( test_parallel_dispatch_lazy_cal_loading ) * added tests for FakeGeneric * This commit just reformats test_transpiler.py fake_generic.py and test_fake_generic.py * Update qiskit/providers/fake_provider/fake_generic.py Co-authored-by: atharva-satpute <[email protected]> * Update qiskit/providers/fake_provider/fake_generic.py Co-authored-by: atharva-satpute <[email protected]> * Update test/python/providers/fake_provider/test_fake_generic.py Co-authored-by: atharva-satpute <[email protected]> * Update test/python/providers/fake_provider/test_fake_generic.py Co-authored-by: atharva-satpute <[email protected]> * Fix some tests * Add calibration schedules, fix transpiler tests * Fix lint * Refactor, add pulse functionality * Latest updates to FakeGeneric * Fix some unit tests * Fix some unit tests * Fix tests, lint, refactor * Remove unused inputs * Make calibrations optional * Attempt to speed up test: only add calibrations if option enabled * Revert some tests to V1 * Refactor FakeGeneric * Update unit test * Rename supported instructions to operations, fix tests Fix test seed Fix unit test, lint * Restore vf2postlayout test to V1 Fx vf2 test * Add pulse test to validate pulse capabilities of new fake backend Update fake generic * Update docs, fix lint * Avoid set for basis gates to allow reproducibility in the error/duration value generation * Update calibrations * Fix transpiler test * Add run test to confirm noise defaults * Add CZ to basis gates, dtm, update backend names, fix docs * Remove GenericTarget, add default for basis_gates, remove supported_operations. * Apply review comments, modify calibrate_instructions to avoid public calibration method. * New name proposal * Fix lint and test * Fix comment * Apply Kevin's suggestions to extract defaults from class * Make defaults private * Fix lint * Revert previous 3 comments. * Privatize GenericFakeBackend * Remove from docs * Revert privatization * Apply review comments * Add reno * Fix lint * Apply suggestions from Matt's code review Co-authored-by: Matthew Treinish <[email protected]> * Apply comments from code review * Fix lint * Rename to GenericBackendV2 and adjust docs. * Fix lint * Apply comments from code review * It's not 2023 anymore * Fix conflict * Fix test conflict. --------- Co-authored-by: atharva-satpute <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: Kevin Hartman <[email protected]> (cherry picked from commit b4582a9) # Conflicts: # qiskit/providers/fake_provider/__init__.py # test/python/compiler/test_transpiler.py
* Add fake generic and modify tests (#10266) * Added a FakeGeneric BackendV2 backend, this is just a bare minimum working code * All FakeBackends (like FakeMelbourne, FakeBoeblingen ,etc) is replaced with FakeGeneric * relocated FakeGeneric in fake_provider directory; replaced if-elif with instruction_dict in fake_generic.py; added support of grid type of coupling map; using lower number of qubits, passed all tests of test_transpiler except one method ( test_parallel_dispatch_lazy_cal_loading ) * added tests for FakeGeneric * This commit just reformats test_transpiler.py fake_generic.py and test_fake_generic.py * Update qiskit/providers/fake_provider/fake_generic.py Co-authored-by: atharva-satpute <[email protected]> * Update qiskit/providers/fake_provider/fake_generic.py Co-authored-by: atharva-satpute <[email protected]> * Update test/python/providers/fake_provider/test_fake_generic.py Co-authored-by: atharva-satpute <[email protected]> * Update test/python/providers/fake_provider/test_fake_generic.py Co-authored-by: atharva-satpute <[email protected]> * Fix some tests * Add calibration schedules, fix transpiler tests * Fix lint * Refactor, add pulse functionality * Latest updates to FakeGeneric * Fix some unit tests * Fix some unit tests * Fix tests, lint, refactor * Remove unused inputs * Make calibrations optional * Attempt to speed up test: only add calibrations if option enabled * Revert some tests to V1 * Refactor FakeGeneric * Update unit test * Rename supported instructions to operations, fix tests Fix test seed Fix unit test, lint * Restore vf2postlayout test to V1 Fx vf2 test * Add pulse test to validate pulse capabilities of new fake backend Update fake generic * Update docs, fix lint * Avoid set for basis gates to allow reproducibility in the error/duration value generation * Update calibrations * Fix transpiler test * Add run test to confirm noise defaults * Add CZ to basis gates, dtm, update backend names, fix docs * Remove GenericTarget, add default for basis_gates, remove supported_operations. * Apply review comments, modify calibrate_instructions to avoid public calibration method. * New name proposal * Fix lint and test * Fix comment * Apply Kevin's suggestions to extract defaults from class * Make defaults private * Fix lint * Revert previous 3 comments. * Privatize GenericFakeBackend * Remove from docs * Revert privatization * Apply review comments * Add reno * Fix lint * Apply suggestions from Matt's code review Co-authored-by: Matthew Treinish <[email protected]> * Apply comments from code review * Fix lint * Rename to GenericBackendV2 and adjust docs. * Fix lint * Apply comments from code review * It's not 2023 anymore * Fix conflict * Fix test conflict. --------- Co-authored-by: atharva-satpute <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: Kevin Hartman <[email protected]> (cherry picked from commit b4582a9) # Conflicts: # qiskit/providers/fake_provider/__init__.py # test/python/compiler/test_transpiler.py * Update __init__.py * Update test_transpiler.py * Update test_transpiler.py * Fix conflicts (#11673) * Rename test, switch to basic simulator * Fix bug in pulse default generation --------- Co-authored-by: MozammilQ <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]>
Summary
(1/4) --> This is the first PR of the
FakeBackend
s refactoring epic.This PR adds a
FakeGeneric(BackendV2)
backend for qiskit unit testing purposes. It only updates the uses of V2FakeBackends
intest/python/compiler/test_transpiler.py
. The rest of the unit tests using V2 fake backends are updated in #10918.Details and comments
FakeGeneric
#10918Note: I can move the
GenericTarget
class to an independent file once the approach is agreed upon and I am still not 100% convinced by theFakeGeneric
name for the backend.