Use transpile to build circuit only once

[RonMeiburg]

Despite being wrapped up in a lambda function, the get_circuit function is actually still called for every function evaluation during plot generation or optimization, and hence the circuit is rebuilt each time. This rather defeats the concept of late binding of the parameter values. The PR uses a slightly different approach using the transpile function. The code is arguably more transparent than using the lambda function wrapper. Evaluation is faster now, but for this simple case rarely more than 10%. One downside, the circuit cannot be plotted anymore in a simple way.

[mstechly]

A couple of comments:

1. You changed the get_circuit function so that it returns energy – this definitely requires some refactoring before merging is possible :)
2. Could you check if using partial instead of lambda helps?
3. It makes code less modular and uses "function inside a function" which in my opinion is actually less transparent ;)
4. Speed is not really the focus in this tutorial, so I'm not sure how much we should really worry about it...
5. . I'm not sure what's the etiquette for PRs to jupyter notebooks, but you've also committed a lot of code coming just from your execution. I guess that's fine in the grand scheme of things, though it makes the PR harder to review. I'd suggest using a tool like SourceTree that allows you to select which lines exactly you want to commit :)

@MSRudolph, thoughts?

[mstechly]

Also, I've rebased the PR so that it points to dev branch instead of main. It shouldn't affect this code though. Adding note about it to the contribution guidelines.

[RonMeiburg]

1. Apologies, was not the intention to make things more difficult
2. Will do. Uncharted territory, but good challenge
3. The use of 'arguably' in the PR had a reason :-)
4. I assumed, and of course I should not, that the use of late binding was for speed reasons. Otherwise why use it? It delivers more complex code than direct insertion of the parameters in the circuit (arguably, my bias!) . Also it would make sense to create demo code, that is likely to be copied and pasted, that gives the best run times.
5. My fault, I'll run nb-clean in future so all execution content is removed.

[MSRudolph]

I see what the point is of this PR, thank you @RonMeiburg. In the last months, I have come to dislike lambda functions, because they evaluate at runtime, rather than when defined. Even so, we build the qiskit circuit in this way simply because we were trying to stay consistent across the different frameworks. With qiskit specifically, your suggestion does indeed work. Since the performance improvements are so minor for the two qubit example, I wonder if it is worth changing it and diverging from the structure of the other notebooks.

[codecov[bot]]

Codecov Report

Merging #29 (fd3e345) into dev (69d3850) will increase coverage by 0.62%. The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev      #29      +/-   ##
==========================================
+ Coverage   81.81%   82.44%   +0.62%     
==========================================
  Files          26       26              
  Lines         638      638              
  Branches       64       68       +4     
==========================================
+ Hits          522      526       +4     
+ Misses         95       92       -3     
+ Partials       21       20       -1     

Impacted Files	Coverage Δ
src/orqviz/gradients.py	88.88% <0.00%> (+14.81%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 69d3850...fd3e345. Read the comment docs.

[mstechly]

The argument in point 4 from @RonMeiburg is a good point – examples should provide the best possible implementation, cause people will be copy-pasting it. If using partial doesn't help, I'll look into it next week, let me know @RonMeiburg !

[RonMeiburg]

Just updated the PR. As per @mstechly request I removed the function returning function construction in the qiskit example, and used partial to replace the lambda wrapper. I think it is much cleaner now and closer to the original source, yet still gives the advantage of using a single transpile call. As an upside the circuit plotting call works again, so that is reintroduced. To maintain style I also added a modified cirq nb with lambda replaced with partial, which @MSRudolph might like. Also ran a full nb-clean on both nb's, so execution output should be removed. Note that the repo actually holds fully executed nb's so you should diff after running an nb-clean on those as well. I would hope more complex circuits benefit from the change. I did anyway, as I had a few useful learnings :-)

[mstechly]

Cool, thanks @RonMeiburg! If you don't mind I'd ask you to actually execute both notebooks once and save the output. Thanks to this users will be able to actually see the output of the notebooks when they just browse GitHub. If you do mind, we can merge this in and I can make another PR, I just don't want to put changes on your fork :)

[RonMeiburg]

@mstechly I created another PR from a new branch with the run once notebooks you requested. I am not 100pct sure this is what works for you. I apologize if I missed the mark. Note I did also update the cell with the version annotation for the various packages to be in line with those used for execution. These are not minimum requirements.

[mstechly]

@RonMeiburg I'll merge this first and then deal with the other PR! @MSRudolph any other comments before we merge?

[MSRudolph]

Thank you for you effort, @RonMeiburg! The qiskit notebook looks great and even follows our current style. It also highlights how you would slightly adapt our examples if your framework of choice required (or allowed for) small clever modifications.