-
Notifications
You must be signed in to change notification settings - Fork 328
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
Added max_circuits_per_job and removed deepcopy dependency of the quantum kernel trainer fixing #701 and #600 #772
Added max_circuits_per_job and removed deepcopy dependency of the quantum kernel trainer fixing #701 and #600 #772
Conversation
Number of maximum circuits for backend added to solve qiskit-community#701.
…nternally,-fails-with-runtime-primitives Bug#600 qkt deepcopies kernel internally, fails with runtime primitives
Pull Request Test Coverage Report for Build 8062887234Details
💛 - Coveralls |
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.
You should add unit tests to cover new features
As I stated here #701 (comment) the primitive ought to be taking care of this as far as I know. I will check, there has been a change where the circuit now has to be an ISA circuit (already conforms to backend in terms of layout and basis gates) instead of allowing logical circuits. So given such knowledge perhaps the limit is expected to be managed too now. |
Also it only needs one release note that lists two fixes. The issues part is for if the release contains known issues that the user should be aware of and workaround - in this case you are fixing issues/bugs so it just needs a fixes section and both can be be specified in the one release note. |
BTW - you can reference the issues in a way that will close them when this PR is merged. So for instance had you written |
…2e5b986c1be.yaml Small release note bugfix
The number of max circuits per job needs to be set in the quantum fidelity kernel for the
Sorted, I hope it is better now.
Added new unit tests to cover them. |
...senotes/notes/fix-701-max_circuits_per_job-and-600-deepcopy-dependency-e6eda2e5b986c1be.yaml
Outdated
Show resolved
Hide resolved
I added
stable backport potential
|
I made the requested changes. |
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 see a problem with creating a new instance. In my understanding such an instance may be quite different from the original one.
qiskit_machine_learning/kernels/algorithms/quantum_kernel_trainer.py
Outdated
Show resolved
Hide resolved
...senotes/notes/fix-701-max_circuits_per_job-and-600-deepcopy-dependency-e6eda2e5b986c1be.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.
Looks good to me. Thank you.
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.
Green light from me too.
…ntum kernel trainer fixing #701 and #600 (#772) * Added an option for num_circuits per job for kernels to fix #701 * Updated documentation and format the style. * Removed deepcopy dependency in quantum_kernel_trainer.py * Added release notes * quick fix for spell test * Added unit tests for max_circuits_per_job * Update fix-701-max_circuits_per_job-and-600-deepcopy-dependency-e6eda2e5b986c1be.yaml Small release note bugfix * Update fix-701-max_circuits_per_job-and-600-deepcopy-dependency-e6eda2e5b986c1be.yaml * Minor modifications for the unit test * Removed copy of TrainableKernel --------- Co-authored-by: oscar-wallis <[email protected]> (cherry picked from commit 2f49e9e)
…ntum kernel trainer fixing #701 and #600 (#772) (#780) * Added an option for num_circuits per job for kernels to fix #701 * Updated documentation and format the style. * Removed deepcopy dependency in quantum_kernel_trainer.py * Added release notes * quick fix for spell test * Added unit tests for max_circuits_per_job * Update fix-701-max_circuits_per_job-and-600-deepcopy-dependency-e6eda2e5b986c1be.yaml Small release note bugfix * Update fix-701-max_circuits_per_job-and-600-deepcopy-dependency-e6eda2e5b986c1be.yaml * Minor modifications for the unit test * Removed copy of TrainableKernel --------- Co-authored-by: oscar-wallis <[email protected]> (cherry picked from commit 2f49e9e) Co-authored-by: M. Emre Sahin <[email protected]>
Summary
Bug fixes have been made to fix issues #701 and #600.
Changes made:
#701: Added a max_circuits_per_job parameter to fidelity quantum kernel used in the case that if more circuits are submitted than the job limit for the backend, the circuits are split up and ran through separate jobs. Documentation was added and formatted for this.
#600: Removed quantum kernel trainers dependency on copy.deepcopy that was throwing an error with real backends. Now initialise a new instance of a class and assign parameters manually. As change is purely internal, no new documentation required.
Details and comments
All relevant unit tests were run on code.
✅ I have updated the documentation accordingly.
✅ I have read the CONTRIBUTING document.
✅ I have added notes to the CHANGELOG.