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

Enable testing of quimb-dependent notebook #6461

Merged
merged 9 commits into from
Feb 14, 2024

Conversation

pavoljuhas
Copy link
Collaborator

Enable Contract-a-Grid-Circuit.ipynb in notebook tests

Fixes #6088

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@CirqBot CirqBot added the Size: XS <10 lines changed label Feb 13, 2024
@pavoljuhas
Copy link
Collaborator Author

Positively confirmed that Contract-a-Grid-Circuit.ipynb is CI tested -

https://github.com/quantumlib/Cirq/actions/runs/7894131175/job/21544168715?pr=6461#step:5:272

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (adf5155) 97.82% compared to head (1251781) 97.81%.
Report is 1 commits behind head on main.

❗ Current head 1251781 differs from pull request most recent head 7024da5. Consider uploading reports for the commit 7024da5 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6461      +/-   ##
==========================================
- Coverage   97.82%   97.81%   -0.01%     
==========================================
  Files        1115     1115              
  Lines       97448    97448              
==========================================
- Hits        95327    95323       -4     
- Misses       2121     2125       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pavoljuhas pavoljuhas marked this pull request as ready for review February 13, 2024 23:44
@pavoljuhas pavoljuhas requested review from vtomole, cduck and a team as code owners February 13, 2024 23:44
@pavoljuhas pavoljuhas marked this pull request as draft February 14, 2024 01:05
Check if the notebooks-stable CI test is failing there.
Also list all installed packages in the notebook environment.
Copy link
Collaborator

@viathor viathor left a comment

Choose a reason for hiding this comment

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

This is still a draft, but it LGTM anyway.

@@ -402,7 +404,7 @@
"metadata": {},
"outputs": [],
"source": [
"ccq.tensor_expectation_value(circuit, ZZ)"
"# ccq.tensor_expectation_value(circuit, ZZ)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this intentional? Why not remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was a temporary change to check if that call causes CI failure in https://github.com/quantumlib/Cirq/actions/runs/7894234524/job/21545039838.
It indeed did, because the CI passed for 1251781.
I have also run the notebook in a stock colab where the last call crashed with out-of-memory.
Testing with a pre-release cirq works, because it has a pinned quimb version.

I am reverting the notebook to its mainline version and excluding it from tests with stable cirq.

Note: CI passes with the last cell commented.
A fresh Colab run confirms that it causes out-of-memory for stable cirq.

This reverts commit 1251781.
Requires a pinned version of quimb specified after last release.
Otherwise the tensor_expectation_value() in the last-cell causes OOM.
Also add the required note text to the notebook.
@CirqBot CirqBot added the size: S 10< lines changed <50 label Feb 14, 2024
@pavoljuhas pavoljuhas marked this pull request as ready for review February 14, 2024 21:29
@pavoljuhas pavoljuhas enabled auto-merge (squash) February 14, 2024 21:48
@pavoljuhas pavoljuhas merged commit 6a40da5 into quantumlib:main Feb 14, 2024
32 checks passed
@pavoljuhas pavoljuhas deleted the enable-quimb-notebook branch February 14, 2024 22:03
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
* Enable Contract-a-Grid-Circuit.ipynb in notebook tests,
  but test it only with the pre-release Cirq.

The notebook requires a pinned version of quimb from quantumlib#6438 otherwise the
tensor_expectation_value() call in the last-cell causes out-of-memory error.

Fixes quantumlib#6088
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50 Size: XS <10 lines changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TODO tasks to follow the next release 1.1.1
3 participants