-
Notifications
You must be signed in to change notification settings - Fork 325
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
updated QNN tutorials to demonstrate usage of QNNCircuit class #664
updated QNN tutorials to demonstrate usage of QNNCircuit class #664
Conversation
code style adjustments w. black minor spelling adjustment style adjustment w. black
docs/conf.py
Outdated
nbsphinx_timeout = 360 | ||
nbsphinx_timeout = 600 |
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 don't increase, first you should figure out why the tutorial is timing out.
"# we decompose the circuit for the QNN to avoid additional data copying\n", | ||
"qnn = EstimatorQNN(\n", | ||
" circuit=circuit.decompose(),\n", | ||
" circuit=circuit,\n", |
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 am not sure of the statement around decompose and additional data copying - I would check this in regards of performance by adding back decompose and see what happens. It maybe that this decomposition speeds things up around binding parameters - maybe internally some copies are avoided during binding this way. Just so happens that a PR was opened in Qiskit earlier today, that is likely related, to flatten off these blocks in NLocal circuits as it leads to way faster binding. I imagine this is more or less what decompose was used here 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.
@woodsp-ibm thanks for your insights!
@adekusar-drl I will try to figure out what caused the long eval times and set back the nbsphinx timeout to its original value :).
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.
FYI: related to decompose above (has a plot of times as comparison)
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.
A small increase in execution time was observed when decomposing the QNNCircuit (several times - using reps =2,3,4). The respective execution time was still much longer than the original version. This is most definitely linked to the issue @woodsp-ibm referred to.
For now, the QNNCircuit class will be demonstrated in tutorials: 02 & 10.
@adekusar-drl can you open another issue to demonstrate the usage of the QNNCircuit
class once the issue Qiskit/qiskit-terra#10282 is closed. I could then pick up the adjustments for tutorials 05 & 11.
I have adjusted the PR summary to reflect the linked issue and its effects on this PR.
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.
From my perspective its curious that something which should be more or less identical to the circuit that was formed separately before here behaves differently - is this the case elsewhere too, ie that the performance of using QNNCircuit elsewhere is worse too, just that here it got noticed. The only thing that occurs to me is that the combined circuit before was a plain QuantumCircuit that contained the composed BlueprintCircuit's (i.e feature map and ansatz) whereas QNNCircuit is a BlueprintCircuit itself. Whatever the difference it maybe that the changes going on in Qiskit improve things in any regards making the decompose hopefully unneeded and maybe even making things more comparable. Anyway I just figured I'd mention this since ideally things ought to be better understood so as QNNCircuits use as a convenience is not adversely impacting performance more generally but probably that's best left to after whatever changes Qiskit brings to the parameter binding.
…ons; Updated wording in tutorial 2 & 10.
Pull Request Test Coverage Report for Build 5309224048
💛 - 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.
Looks good, thanks!
* updated tutorials to demonstrate usage of QNNCircuit class code style adjustments w. black minor spelling adjustment style adjustment w. black * using Python3 Kernel insted of custom qiskit_ml_env Kernel * increasing nbsphinx_timeout to extend build time for tutorial 05 * rollback changes in tutorial 5 & 11 due to build performance limitations; Updated wording in tutorial 2 & 10.
Summary
The usage of the recently introduced
QNNCircuit
class is showcased in the tutorialsto demonstrate the simplified interface of composing a parameterized quantum circuit from a feature map and an ansatz.
Closes #613
Details and comments
Usage of the
QNNCircuit
in the tutorialsleads to an increase in the execution time. Demonstrating the usage of the
QNNCircuit
class in these notebooks will be picked up after the issue causing the performance slowdown Qiskit/qiskit-terra/#10282 is closed.The QNNCircuit class is intentionally not introduced in tutorials 01, 02a, 03, 04, 06, 07, 08, 09, and 12, because either one of the below reasons apply:
QNNCircuit
class would not simplify the code/usage.QNNCircuit
class.