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

Migrate entanglement forging to Qiskit Nature 0.5.0 #83

Merged
merged 63 commits into from
May 5, 2023

Conversation

caleb-johnson
Copy link
Collaborator

@caleb-johnson caleb-johnson commented Apr 5, 2023

Fixes #89
Fixes #94

This PR removes the deprecated Qiskit Nature API calls from the entanglement forging source code.

This PR removes support for deprecated Qiskit Nature classes as inputs. See the release note for more details.

Considerations/Stray Thoughts:

  • The EF with quantum serverless notebook tutorial is breaking on latest/dev branches in CI due to an incompatibility between the new ElectronicStructureProblem class in Nature 0.6.0 and quantum serverless. The serverless folks probably need to add support for a new class or two to support serialization of the new classes coming from Nature. I've made an issue here. I've turned off testing of this notebook for now. I think that's the best we can do until a QServerless fix is available.

Tasks before merging:

  • Make issue for adding the serverless tutorial back to the development workflow in CI when quantum serverless fix is available.

  • Ensure MO coeff handling is done in the most reasonable way. Work with Nature devs to review.

  • Update rst files in docs with new Nature API calls

  • merge in latest main and revert Temporarily disable qiskit-nature development version test #120 on this branch

@garrison
Copy link
Member

garrison commented Apr 6, 2023

There is also #59.

@caleb-johnson caleb-johnson marked this pull request as ready for review April 7, 2023 23:29
@caleb-johnson caleb-johnson changed the title WIP: Remove deprecated Nature API calls Remove deprecated Nature API calls Apr 7, 2023
@coveralls
Copy link

coveralls commented Apr 8, 2023

Pull Request Test Coverage Report for Build 4702934575

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 39 of 45 (86.67%) changed or added relevant lines in 2 files are covered.
  • 16 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.5%) to 84.088%

Changes Missing Coverage Covered Lines Changed/Added Lines %
circuit_knitting_toolbox/entanglement_forging/cholesky_decomposition.py 17 18 94.44%
circuit_knitting_toolbox/entanglement_forging/entanglement_forging_ground_state_solver.py 22 27 81.48%
Files with Coverage Reduction New Missed Lines %
circuit_knitting_toolbox/entanglement_forging/cholesky_decomposition.py 8 93.15%
circuit_knitting_toolbox/entanglement_forging/entanglement_forging_ground_state_solver.py 8 78.65%
Totals Coverage Status
Change from base Build 4689211493: -0.5%
Covered Lines: 1448
Relevant Lines: 1722

💛 - Coveralls

@garrison
Copy link
Member

garrison commented Apr 14, 2023

I would prefer removing the serverless notebook to delaying this. Qiskit Nature 0.6.0 has already been released, and there are more updates we need to make to the source code to leverage the new types in 0.6.0 and clean some more things up. I'd prefer to get started on that. Running EF with quantum serverless is not a key feature of CKT, just a feature/compatibility. When they suppor the algorithm, we can replace the tutorial.

Yes, that's the other option that allows us to avoid breaking the tutorials. 🙂

I'd be fine with removing that notebook in a subsequent PR so that we can easily revert that PR when the bug is fixed. I'm sure we could also figure out restoring it if made part of this PR, too.

garrison added a commit that referenced this pull request Apr 28, 2023
Now that qiskit-community/qiskit-nature#1142 has been merged, our "development version tests" are going to fail on `main` until #83 is merged.  This change fixes CI immediately by excluding the development version of qiskit-nature from these tests.  This change should be reverted as part of #83 before it is merged.
garrison added a commit that referenced this pull request Apr 28, 2023
Now that qiskit-community/qiskit-nature#1142 has been merged, our "development version tests" are going to fail on `main` until #83 is merged.  This change fixes CI immediately by excluding the development version of qiskit-nature from these tests.  This change should be reverted as part of #83 before it is merged.
@garrison garrison added this to the 0.1.0 milestone May 4, 2023
tox.ini Outdated
@@ -35,7 +35,7 @@ deps =
extras =
notebook-dependencies
commands =
pytest --nbmake --nbmake-timeout=3000 {posargs} docs/
pytest --nbmake --nbmake-timeout=3000 {posargs} --ignore-glob=*forging*serverless* docs/
Copy link
Member

@garrison garrison May 5, 2023

Choose a reason for hiding this comment

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

I think we should put a notice at the top of this notebook, saying that it is currently broken and point to issue #108.

Copy link
Collaborator Author

@caleb-johnson caleb-johnson May 5, 2023

Choose a reason for hiding this comment

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

Will do, will include #413 from serverless as well

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 tutorial works now. I updated the code, and Paul had also updated the serverless usage. I'm not sure what fixed it, because the error I encountered before implementing my fix was different than before.

Anyway, I'm returning this to its original state

Copy link
Member

Choose a reason for hiding this comment

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

Haha, this is great ! 😁

Copy link
Collaborator Author

@caleb-johnson caleb-johnson May 5, 2023

Choose a reason for hiding this comment

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

Qiskit/qiskit-serverless#413 (comment)

Yes! I updated the serverless issue here, and I'll probably recommend they close it soon when this is passing CI

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 is still broken in 0.6.0 but is fixed in 0.5.0. I think I will move the ignore-glob into the Actions workflow and out of tox, since it only affects the one workflow (I think/hope)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will add a note to the serverless notebook as discussed before

Copy link
Member

Choose a reason for hiding this comment

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

We could consider pinning Qiskit nature < 0.6 in the notebook-dependencies only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could consider pinning Qiskit nature < 0.6 in the notebook-dependencies only.

I like that. Let's do that instead of ignoring the notebook, and I'll leave the warning in the notebook for now?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds perfect to me (as long as CI passes 😂). In particular, I think it makes sense to keep the warning in the notebook, in case people are using the tutorial in a setting where they did not install using [notebook-dependencies].

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.

🎉 🥳

@caleb-johnson caleb-johnson merged commit c0bf83a into main May 5, 2023
@caleb-johnson caleb-johnson deleted the remove-deprecated-nature-code branch May 5, 2023 19:43
garrison added a commit that referenced this pull request Nov 6, 2023
garrison added a commit that referenced this pull request Nov 6, 2023
garrison added a commit that referenced this pull request Nov 6, 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.

Migrate to Qiskit Nature 0.5.0 Remove IntegralDriver from Entanglement Forging
4 participants