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

Add Migration Guide for Opflow #9549

Merged
merged 56 commits into from Apr 6, 2023
Merged

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Feb 8, 2023

Summary

This PR closes #9484
Blocked by #9716 (doctest & intersphinx config) Copied over docs config from #9716 (agreed upon with @HuangJunye)

Details and comments

Open Questions:

  • Should I remove the links to the opflow API ref?
  • Any formatting improvements?
  • Any examples that you miss?

TO-DOs:

  • Add StateFn and expectations examples
  • Add links for gradient section (tutorial still not created)
  • Add hyperlinks to other sections when mentioned (see ... example type of references)
  • Make explanation for evolutions more concise

@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@ElePT ElePT self-assigned this Feb 8, 2023
@coveralls
Copy link

coveralls commented Feb 8, 2023

Pull Request Test Coverage Report for Build 4617374131

  • 3 of 3 (100.0%) changed or added relevant lines in 3 files are covered.
  • 5 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.03%) to 85.401%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/vf2_layout.rs 1 86.44%
qiskit/circuit/tools/pi_check.py 1 91.23%
qiskit/pulse/library/waveform.py 3 91.67%
Totals Coverage Status
Change from base Build 4617156725: -0.03%
Covered Lines: 67458
Relevant Lines: 78990

💛 - Coveralls

@HuangJunye HuangJunye self-assigned this Feb 8, 2023
@HuangJunye HuangJunye added the documentation Something is not clear or an error documentation label Feb 8, 2023
Copy link
Contributor

@ikkoham ikkoham left a comment

Choose a reason for hiding this comment

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

Thank you for great work! I left some comments.

docs/migration_guides/opflow_migration.rst Outdated Show resolved Hide resolved
docs/migration_guides/opflow_migration.rst Outdated Show resolved Hide resolved
docs/migration_guides/opflow_migration.rst Outdated Show resolved Hide resolved
@woodsp-ibm woodsp-ibm added the mod: opflow Related to the Opflow module label Feb 8, 2023
docs/migration_guides/opflow_migration.rst Outdated Show resolved Hide resolved
docs/migration_guides/opflow_migration.rst Outdated Show resolved Hide resolved
docs/migration_guides/opflow_migration.rst Outdated Show resolved Hide resolved

.. code-block:: python

from qiskit.opflow import PuliSumOp
Copy link
Member

Choose a reason for hiding this comment

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

I think Matthew said someway about running/testing these samples. Note sure if thats doctest - we have used that in apps. (Reason I comment here is due to typo in import - PuliSumOp no a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@Cryoris Cryoris Feb 13, 2023

Choose a reason for hiding this comment

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

I'm pretty sure these are not tested at the moment, unless they are in a jupyter-execute environment 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So @woodsp-ibm, I managed to find an inbetween solution, I formatted the code examples to be runnable locally with pytest's doctest. I don't know if I like having all of the >>> in the code, but at least we can make sure that it runs.

Copy link
Member

Choose a reason for hiding this comment

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

It is also possible to use ..testcode:: and ..testoutput:: directives (they are supported by the sphinx.ext.doctest, which we use in the apps and imagine would be used here too). By default the output puts these in separate highlighted blocks (though you can hide the testoutput if desired) but there is no need for >>> when using these.

I know though we used >>> in Nature etc since formatting with these former seemed harder to get things to come out right/as wanted etc. The >>> was always more concise and seemed easier to add and have it come out as expected in among other text. Maybe there is styling that can applied to testcode/output so things can be improved, not sure.

E.g.

.. testcode::

    x = 1 + 3
    print(x)

.. testoutput::

    4

comes out as
image

docs/migration_guides/opflow_migration.rst Outdated Show resolved Hide resolved
docs/migration_guides/opflow_migration.rst Outdated Show resolved Hide resolved
docs/migration_guides/opflow_migration.rst Outdated Show resolved Hide resolved
@woodsp-ibm
Copy link
Member

Should I remove the links to the opflow API ref?

If this is published along with the API ref and by kept version, as the docs currently are, then this migration guide could be removed when opflow is removed. If not then afterwards the class refs will just be highlighted - just not links since they would not resolve. To me having them as class refs so they can be links seems better - since as links I can go to the opflow class itself for more detail if I need that.

@ElePT ElePT marked this pull request as ready for review February 13, 2023 16:29
@ElePT ElePT requested a review from a team as a code owner February 13, 2023 16:29
@ElePT ElePT requested a review from Cryoris February 13, 2023 16:29
qiskit/quantum_info/__init__.py Outdated Show resolved Hide resolved
HuangJunye
HuangJunye previously approved these changes Apr 4, 2023
Copy link
Collaborator

@HuangJunye HuangJunye left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot for working on this!

docs/index.rst Show resolved Hide resolved
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

LGTM -- only one more left! 😄

@Cryoris Cryoris added this pull request to the merge queue Apr 6, 2023
Merged via the queue into Qiskit:main with commit 10db7bf Apr 6, 2023
13 checks passed
giacomoRanieri pushed a commit to giacomoRanieri/qiskit-terra that referenced this pull request Apr 16, 2023
* Add draft

* Remove qiskit

* Apply review

* Apply comments boxnote

* Small edit

* Apply review

* Update up to primitive ops

* Dump changes from sphinx repo

* Add twisties

* Update information, add examples

* Formatting changes

* Simplify intro

* Add links

* Add sampler example

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Apply suggestions from code review

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update CircuitOp

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Applied suggestions locally

* Change format, links

* Add grouping, algorithms link

* Fix indentation

* Final review, add links

* Add gradients

* Apply heading suggestions

Co-authored-by: Junye Huang <h.jun.ye@gmail.com>

* Add tags

* Apply suggestions code review

* Correct code examples, add outputs

* Run code examples by doctest

* Remove underscores

* Apply suggestions second round of review

Co-authored-by: Junye Huang <h.jun.ye@gmail.com>

* Add globals link

* Change links to refs

* Add dropdowns

* Change doctest syntax, apply reviews

* Try CI config

* Add internal links

* Fix lint, add doctest import

* Fix cyclic import

* Fix cyclic import

* Apply suggestions from Declan's code review

Co-authored-by: Declan Millar <declan.millar@ibm.com>

* Update qiskit/quantum_info/__init__.py

* Fix import

* Copy docs config from Qiskit#9716

* Make black

* Remove duplicate

* Reorder

---------

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Junye Huang <h.jun.ye@gmail.com>
Co-authored-by: Declan Millar <declan.millar@ibm.com>
king-p3nguin pushed a commit to king-p3nguin/qiskit-terra that referenced this pull request May 22, 2023
* Add draft

* Remove qiskit

* Apply review

* Apply comments boxnote

* Small edit

* Apply review

* Update up to primitive ops

* Dump changes from sphinx repo

* Add twisties

* Update information, add examples

* Formatting changes

* Simplify intro

* Add links

* Add sampler example

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Apply suggestions from code review

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update CircuitOp

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Applied suggestions locally

* Change format, links

* Add grouping, algorithms link

* Fix indentation

* Final review, add links

* Add gradients

* Apply heading suggestions

Co-authored-by: Junye Huang <h.jun.ye@gmail.com>

* Add tags

* Apply suggestions code review

* Correct code examples, add outputs

* Run code examples by doctest

* Remove underscores

* Apply suggestions second round of review

Co-authored-by: Junye Huang <h.jun.ye@gmail.com>

* Add globals link

* Change links to refs

* Add dropdowns

* Change doctest syntax, apply reviews

* Try CI config

* Add internal links

* Fix lint, add doctest import

* Fix cyclic import

* Fix cyclic import

* Apply suggestions from Declan's code review

Co-authored-by: Declan Millar <declan.millar@ibm.com>

* Update qiskit/quantum_info/__init__.py

* Fix import

* Copy docs config from Qiskit#9716

* Make black

* Remove duplicate

* Reorder

---------

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Junye Huang <h.jun.ye@gmail.com>
Co-authored-by: Declan Millar <declan.millar@ibm.com>
Eric-Arellano pushed a commit to Qiskit/documentation that referenced this pull request Oct 6, 2023
* Add draft

* Remove qiskit

* Apply review

* Apply comments boxnote

* Small edit

* Apply review

* Update up to primitive ops

* Dump changes from sphinx repo

* Add twisties

* Update information, add examples

* Formatting changes

* Simplify intro

* Add links

* Add sampler example

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Apply suggestions from code review

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update CircuitOp

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Applied suggestions locally

* Change format, links

* Add grouping, algorithms link

* Fix indentation

* Final review, add links

* Add gradients

* Apply heading suggestions

Co-authored-by: Junye Huang <h.jun.ye@gmail.com>

* Add tags

* Apply suggestions code review

* Correct code examples, add outputs

* Run code examples by doctest

* Remove underscores

* Apply suggestions second round of review

Co-authored-by: Junye Huang <h.jun.ye@gmail.com>

* Add globals link

* Change links to refs

* Add dropdowns

* Change doctest syntax, apply reviews

* Try CI config

* Add internal links

* Fix lint, add doctest import

* Fix cyclic import

* Fix cyclic import

* Apply suggestions from Declan's code review

Co-authored-by: Declan Millar <declan.millar@ibm.com>

* Update qiskit/quantum_info/__init__.py

* Fix import

* Copy docs config from Qiskit/qiskit#9716

* Make black

* Remove duplicate

* Reorder

---------

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Junye Huang <h.jun.ye@gmail.com>
Co-authored-by: Declan Millar <declan.millar@ibm.com>
Eric-Arellano pushed a commit to Qiskit/documentation that referenced this pull request Oct 9, 2023
* Add draft

* Remove qiskit

* Apply review

* Apply comments boxnote

* Small edit

* Apply review

* Update up to primitive ops

* Dump changes from sphinx repo

* Add twisties

* Update information, add examples

* Formatting changes

* Simplify intro

* Add links

* Add sampler example

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/migration_guides/opflow_migration.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Apply suggestions from code review

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update CircuitOp

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Applied suggestions locally

* Change format, links

* Add grouping, algorithms link

* Fix indentation

* Final review, add links

* Add gradients

* Apply heading suggestions

Co-authored-by: Junye Huang <h.jun.ye@gmail.com>

* Add tags

* Apply suggestions code review

* Correct code examples, add outputs

* Run code examples by doctest

* Remove underscores

* Apply suggestions second round of review

Co-authored-by: Junye Huang <h.jun.ye@gmail.com>

* Add globals link

* Change links to refs

* Add dropdowns

* Change doctest syntax, apply reviews

* Try CI config

* Add internal links

* Fix lint, add doctest import

* Fix cyclic import

* Fix cyclic import

* Apply suggestions from Declan's code review

Co-authored-by: Declan Millar <declan.millar@ibm.com>

* Update qiskit/quantum_info/__init__.py

* Fix import

* Copy docs config from Qiskit/qiskit#9716

* Make black

* Remove duplicate

* Reorder

---------

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Junye Huang <h.jun.ye@gmail.com>
Co-authored-by: Declan Millar <declan.millar@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Something is not clear or an error documentation mod: opflow Related to the Opflow module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Opflow Migration Guide
9 participants