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

Suppress warnings from Qiskit Nature #230

Merged
merged 17 commits into from
Jun 19, 2023
Merged

Conversation

SaashaJoshi
Copy link
Contributor

@SaashaJoshi SaashaJoshi commented Jun 1, 2023

Resolves #154

@coveralls
Copy link

coveralls commented Jun 1, 2023

Pull Request Test Coverage Report for Build 5146937856

  • 8 of 8 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.008%) to 89.375%

Totals Coverage Status
Change from base Build 5127839616: 0.008%
Covered Lines: 2330
Relevant Lines: 2607

💛 - Coveralls

Comment on lines 37 to 40
from qiskit_nature import settings

settings.use_pauli_sum_op = False

Copy link
Member

Choose a reason for hiding this comment

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

Since it may violate a user's expectations to modify global state like this, it deserves some comments here of why it's necessary and also deserves mention in the release notes. (At least it is a temporary thing, and will have no effect on Qiskit Nature versions 0.7 and higher.)

Copy link
Member

Choose a reason for hiding this comment

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

Also, it might be better to move this one level up, to entanglement_forging/__init__.py, for increased visibility.

@garrison
Copy link
Member

garrison commented Jun 6, 2023

When I run this branch under pytest, all versions pass:

  py38: OK (105.50=setup[71.32]+cmd[34.19] seconds)
  py39: OK (129.52=setup[89.50]+cmd[40.02] seconds)
  py310: OK (138.50=setup[100.98]+cmd[37.52] seconds)
  py311: OK (135.71=setup[102.40]+cmd[33.30] seconds)
  congratulations :) (509.33 seconds)

When I run the tests again locally against current development versions, using python tools/extremal_dependency_versions.py pin_dependencies dev --inplace (this is destructive; it modifies pyproject.toml), then all tests pass again as well:

  py38: OK (878.60=setup[865.89]+cmd[12.71] seconds)
  py39: OK (161.98=setup[147.90]+cmd[14.08] seconds)
  py310: OK (152.29=setup[140.42]+cmd[11.88] seconds)
  py311: OK (152.60=setup[142.16]+cmd[10.44] seconds)
  congratulations :) (1345.72 seconds)

When I run notebooks from this branch, they fail on all versions of python:

  py38-notebook: FAIL code 1 (181.68=setup[73.70]+cmd[107.98] seconds)
  py39-notebook: FAIL code 1 (184.47=setup[74.37]+cmd[110.11] seconds)
  py310-notebook: FAIL code 1 (189.66=setup[75.84]+cmd[113.82] seconds)
  py311-notebook: FAIL code 1 (173.32=setup[75.39]+cmd[97.94] seconds)
  evaluation failed :( (729.22 seconds)

So, at the moment, it seems that the tests fail only on the notebook(s), regardless of python version.

@garrison
Copy link
Member

garrison commented Jun 6, 2023

I thought for a moment that fixing this might be as simple as importing the entanglement_forging module before using Qiskit Nature to do anything, but I tried this and it doesn't seem to have fixed anything.

Comment on lines 31 to 34
from qiskit_nature import settings

settings.use_pauli_sum_op = False

Copy link
Member

Choose a reason for hiding this comment

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

These lines actually need to be moved to circuit_knitting/forging/__init__.py now that #244 is merged.

Copy link
Member

Choose a reason for hiding this comment

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

And it'd be great if you could add a comment above it:

From now forward, the entanglement forging module requires that SparsePauliOp be used rather than PauliSumOp, as the latter is deprecated in Qiskit Terra 0.24. However, Qiskit Nature still is not expected to change this default until Qiskit Nature 0.7. Here, we modify the global state to opt into the new behavior early. Unfortunately, this means that any code that calls Qiskit Nature in the same process as entanglement forging will need to be updated to use SparsePauliOp as well.

Copy link
Member

Choose a reason for hiding this comment

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

Also, once we finally figure out the magic to get everything working, we should double check that this block is actually required to get tests to pass. The migration guide suggests that it is not always necessary to change this setting. If you provide a SparsePauliOp to a function that returns an operator, it should result in a SparsePauliOp regardless of this setting, from what I understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was in fact added after a discussion between @caleb-johnson and Max (Rossmannek). He suggested that we can suppress the warnings with these settings until opflow is dropped, which will be in the 0.8.0 release.

@garrison
Copy link
Member

garrison commented Jun 7, 2023

The following line in pyproject.toml is preventing Qiskit Nature 0.6.0 from being used in the notebook tests, but I believe that using SparsePauliOp (this PR) requires getting rid of that upper bound. (At least, there is no attribute setting.use_pauli_sum_op in earlier versions.) That might then cause a failure related to quantum serverless in one of the notebooks, but we can evaluate what to do about that (this is all related to #108).

https://github.com/Qiskit-Extensions/circuit-knitting-toolbox/blob/4a0cd45c5f28b196800aefd843f380ba7136494c/pyproject.toml#L84

The dependency should also be updated at the following line in order for the minimum version tests to pass:

https://github.com/Qiskit-Extensions/circuit-knitting-toolbox/blob/4a0cd45c5f28b196800aefd843f380ba7136494c/pyproject.toml#L31

The README badge should be updated to match that version, too.

@garrison garrison linked an issue Jun 16, 2023 that may be closed by this pull request
garrison
garrison previously approved these changes Jun 16, 2023
Copy link
Member

@garrison garrison left a comment

Choose a reason for hiding this comment

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

This looks great. Let's wait for @caleb-johnson's approval, too.

fwiw, I tried to remove settings.use_pauli_sum_op = False, but tests failed, so it looks like we have to keep it. Not a big surprise, but I wanted to double check that it is absolutely necessary.

@garrison garrison added this to the 0.3.0 milestone Jun 16, 2023
@garrison
Copy link
Member

Oh yes -- I forgot before approving: This certainly deserves a release note, too.

Copy link
Collaborator

@caleb-johnson caleb-johnson left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @SaashaJoshi and @garrison !

I suggested a couple of small simplifications on the import and like Jim suggested, please add a release note covering this change. :)

circuit_knitting/forging/cholesky_decomposition.py Outdated Show resolved Hide resolved
@@ -11,7 +11,7 @@
"\n",
"See [Tutorial 1](tutorial_1_getting_started.ipynb) for a high-level breakdown of the entanglement forging algorithm, or check out the [explanatory material](../explanation/index.rst) for a more detailed explanation.\n",
"\n",
"<span style=\"color:red\"><i>Quantum Serverless does not support Qiskit Nature 0.6.0 yet. In order to use entanglement forging with quantum serverless, users should use 0.5.2 < Qiskit Nature < 0.6.0. There is an [associated issue in the quantum-serverless repo](https://github.com/Qiskit-Extensions/quantum-serverless/issues/413) to address this.</i></span>"
"<span style=\"color:red\"><i>Quantum Serverless does not support Qiskit Nature 0.6.0 yet. In order to use entanglement forging with quantum serverless, users should downgrade to version 0.2 of the Circuit Knitting Toolbox, as it is compatible with Qiskit Nature 0.5.2. There is an [associated issue in the quantum-serverless repo](https://github.com/Qiskit-Extensions/quantum-serverless/issues/413) to address this.</i></span>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice

SaashaJoshi and others added 2 commits June 16, 2023 20:16
Co-authored-by: Caleb Johnson <calebj1524@outlook.com>
…ver.py

Co-authored-by: Caleb Johnson <calebj1524@outlook.com>
Copy link
Member

@garrison garrison left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@@ -0,0 +1,8 @@
---
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice

Copy link
Collaborator

@caleb-johnson caleb-johnson left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@SaashaJoshi SaashaJoshi merged commit 8f40d0a into Qiskit:main Jun 19, 2023
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.

Replace ListOp in EntForging Migrate to Qiskit Nature 0.6.0
4 participants