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

update: Re-enable commented code chunks in several notebooks #372

Merged
merged 22 commits into from
Oct 16, 2023

Conversation

rmshaffer
Copy link
Contributor

@rmshaffer rmshaffer commented Oct 2, 2023

Issue #, if available:

Description of changes:
Reduces runtime of commented code chunks and ensures that all code is uncommented in several example notebooks:

  • examples/braket_features/Using_the_tensor_network_simulator_TN1.ipynb: slightly reduce qubit, gate, and shot count to reduce runtime and cost - new total runtime 2 min (unchanged)
  • examples/hybrid_jobs/7_Running_notebooks_as_hybrid_jobs/Running_notebooks_as_hybrid_jobs.ipynb: show cached results from running jobs on multiple devices, explain how to run on QPUs if desired - new total runtime 4 min (down from 8 min)
  • examples/pennylane/1_Parallelized_optimization_of_quantum_circuits/1_Parallelized_optimization_of_quantum_circuits.ipynb: reduce qubit count, reduce warning about long execution time to ~5 min from ~15 min - new total runtime 0.75 min (unchanged)
  • examples/pennylane/2_Graph_optimization_with_QAOA/2_Graph_optimization_with_QAOA.ipynb: simplify problem, use SV1 adjoint gradient (diff_method="device") to speed up by ~10x - new total runtime 2 min (down from 3 min)
  • examples/pennylane/4_Simulation_of_noisy_quantum_circuits_on_Amazon_Braket_with_PennyLane/4_Simulation_of_noisy_quantum_circuits_on_Amazon_Braket_with_PennyLane.ipynb: reduce problem size and iteration count, display cached results rather than including long execution time, re-run notebook to ensure output matches code - new total runtime 1 min (unchanged)

Note that despite uncommenting the code, this PR does not increase the runtime of the notebooks (because there are some corresponding reductions in problem size where appropriate).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@peterkomar-aws peterkomar-aws self-requested a review October 2, 2023 15:49
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@krneta
Copy link
Contributor

krneta commented Oct 5, 2023

I see a couple of errors in the tests:

QuantumFunctionError: The braket.aws.qubit device does not provide a native method for computing the jacobian.

Is this happening because you're relying on some PL version?

ValueError: Result type not found in result. Result types must be added to circuit before circuit is run on device.

This is likely because our mocks are returning generic results and not the type that you expect. This is the result that we're currently returning for this test (for all calls). Can you check if the result should be modified or new results added?

@rmshaffer rmshaffer changed the title update: Reduce runtime and re-enable commented code chunks in several notebooks update: Re-enable commented code chunks in several notebooks Oct 12, 2023
@rmshaffer rmshaffer marked this pull request as ready for review October 12, 2023 13:29
@rmshaffer rmshaffer requested a review from a team as a code owner October 12, 2023 13:29
@rmshaffer rmshaffer requested a review from krneta October 12, 2023 13:29
@@ -1,400 +1,416 @@
{
Copy link
Contributor

@krneta krneta Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what we're showcasing here? It seems like the big addition here is the for loop around the AwsQuantumJob.create, and that seems like something we shouldn't need to show here.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove the code from this section that runs the multiple jobs - you're right, it's just demonstrating a for loop, there's nothing else new here.

krneta
krneta previously approved these changes Oct 12, 2023
@@ -22,17 +22,20 @@
"cell_type": "markdown",
Copy link
Contributor

@licedric licedric Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #3.    n_nodes = 4

Why does the problem need to be shrunk here?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The optimization run for this problem is done on the LocalSimulator, so shrinking the problem cuts the local runtime roughly in half. Do you feel the demonstration is less valuable/understandable at the smaller size?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, surprised that it makes a big enough difference for the local simulator here between 4 and 6 qubits. Are you sure the time savings isn't coming from the fewer number of iterations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The number of iterations for this example is still 10 - it hasn't been changed here. The problem size is reduced by shrinking the number of nodes (from 6 to 4) and layers (from 4 to 2).

@@ -232,7 +232,7 @@
"outputs": [
Copy link
Contributor

@licedric licedric Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of hard to see convergence here.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll run this out to 25 iterations to make the convergence more obvious on the plot.

@rmshaffer rmshaffer merged commit 0af523b into main Oct 16, 2023
@rmshaffer rmshaffer deleted the rmshaffer/uncomment-code branch October 16, 2023 23:25
ajberdy added a commit that referenced this pull request Oct 16, 2023
@ajberdy
Copy link
Contributor

ajberdy commented Oct 16, 2023

Hey, the example repo is frozen for the jobs decorator launch and this PR introduces conflicts with the feature branch (being merged today). I've reverted it in #414 and restored this branch

@ajberdy ajberdy restored the rmshaffer/uncomment-code branch October 16, 2023 23:37
@rmshaffer rmshaffer deleted the rmshaffer/uncomment-code branch October 17, 2023 01:03
@rmshaffer
Copy link
Contributor Author

Thanks, and sorry about that. I've copied the changes to a new branch rmshaffer/uncomment-code-2 and will make a new PR from there after merging and re-running the conflicting notebook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants