Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.1k stars 2.34k forks source link

Improve ZZFeatureMap API docs #12431

Open Eric-Arellano opened 4 months ago

Eric-Arellano commented 4 months ago

Feedback from Paul Nation:

This page is very out of date: https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.ZZFeatureMap. and some of the code is not even valid anymore. e.g `
ZZFeatureMap(3) + EfficientSU2(3)
raises TypeError: unsupported operand type(s) for +: 'ZZFeatureMap' and 'EfficientSU2'

Also the data, entanglement_blocks, num_parameters, and parameters properties have no docstring or type hints:

Screenshot 2024-05-17 at 10 56 54 AM
nonhermitian commented 4 months ago

It looks like the docs have not been updated in 4 years, and the doc strings are not evaluated to find broken code.

Shivansh20128 commented 1 week ago

Hi! @nonhermitian I want to work on this issue. Can you please assign this to me? Thank you

Shivansh20128 commented 6 days ago

Hi @Eric-Arellano, I have a doubt. The above documentation needs to be edited in this file link. I noticed one error in the documentation (just a start, I can see many actually) here. The given code mentioned in the documentation does not give the circuit diagram that is shown in the documentation, and the line print(prep) should be replaced with print(prep.decompose()).

from qiskit.circuit.library import ZZFeatureMap
prep = ZZFeatureMap(2, reps=1)
print(prep)

Output with print(prep)

     ┌──────────────────────────┐
q_0: ┤0                         ├
     │  ZZFeatureMap(x[0],x[1]) │
q_1: ┤1                         ├
     └──────────────────────────┘

Output with print(prep.decompose()):

     ┌───┐┌─────────────┐                                          
q_0: ┤ H ├┤ P(2.0*x[0]) ├──■────────────────────────────────────■──
     ├───┤├─────────────┤┌─┴─┐┌──────────────────────────────┐┌─┴─┐
q_1: ┤ H ├┤ P(2.0*x[1]) ├┤ X ├┤ P(2.0*(π - x[0])*(π - x[1])) ├┤ X ├
     └───┘└─────────────┘└───┘└──────────────────────────────┘└───┘

When I went to make changes here, I noticed that the line has already been updated, but is not reflecting in the documentation. So my question is, is this the correct file to edit to update the documentation? Thank you

Eric-Arellano commented 6 days ago

Hey @Shivansh20128 , the difference is the dev version of the docs (corresponding to the main branch) vs the 1.2 version of the docs (corresponding to the stable/1.2 branch). You can see the above updates reflected at https://docs.quantum.ibm.com/api/qiskit/dev/qiskit.circuit.library.ZZFeatureMap.

You'll want to improve the dev version of the docs. So, yes, you'll be changing _zz_feature_map.py. The dev docs will become the stable docs once Qiskit 1.3 is released.

Shivansh20128 commented 5 days ago

okay, I got it. Thank you @Eric-Arellano But now I have a follow-up question. I made a PR some days back about a typo link, which has been merged now. But the changes are not reflected even in the dev version of the docs as you can see here. According to my PR, the typo "you cannot use a gate in a definition it its own definition depends on the parent." should be corrected to "you cannot use a gate in a definition if its own definition depends on the parent.".

Also, is there a way to see a preview of the documentation I am making (like how it would look on the website)? Because the changes I am making are in a .py and not a .md file, I cannot see if it looks the way I want it to.

Thank you

Shivansh20128 commented 5 days ago

@Eric-Arellano I have created a pull request link with the required documentation for issue. I think there is some problem as my first PR has not been reviewed and merged yet. It is also based on documentation. Please review it and close this issue. Thank You

Eric-Arellano commented 5 days ago

But the changes are not reflected even in the dev version of the docs as you can see here.

Merging to main is only the first step to updating the live dev docs. We then need to regenerate the documentation in https://github.com/Qiskit/documentation to convert from Sphinx HTML files to MDX files understood by the docs website. That used to be a manual process, but we automated it yesterday to be a nightly cron job. Then, once the update is merged in Qiskit/documentation, a maintainer has to deploy the docs live. So, when you make a change to qiskit/qiskit docs, you can expect a 1-3 day delay before it goes live to /dev.

I deployed your change this morning so it is now live.

is there a way to see a preview of the documentation I am making

You can run tox -e docs locally, then look at docs/_build/html and open up the HTML files in your browser. Sometimes Tox gets into a bad cache state, so it can help to run tox -e docs-clean.

This preview is of Sphinx's output. To preview the actual website, you can follow this guide https://github.com/Qiskit/documentation#api-docs-authors-how-to-preview-your-changes. However, you almost never need to do that and that is much slower and more complex to do. Most people only do tox -e docs and look at the Sphinx HTML page.

I think there is some problem as my first PR has not been reviewed and merged yet. It is also based on documentation.

There is no issue. In open source, it is normal to take some time for people to review because there are not enough maintainers to keep up with everything promptly. Please be patient.

And so I cannot delete my forked repo.

That shouldn't matter. It's common for people to keep forks around, and it can be useful to show that you're engaged in open source.

@jakelishman Please review this pull request and let me know if its alright.

No need to explicitly ask for the review. Jake is already auto-added to the review as part of terra-core. He's quite busy and will get to it when he has time.

Cryoris commented 5 days ago

Some comments and questions:

Shivansh20128 commented 4 days ago

Hi @Cryoris , If a docstring is not explicitly overriden in an inherited class, then it will not automatically inherit the docstring spedified in the parent class. So each inherited class needs to have its own docstring defined. You can find an example here under the "Docstrings for Python Classes" section. I hope it helps.

Also, I did not understand what you meant by the first comment. Which docstring code examples are you talking about? Thank you

Shivansh20128 commented 4 days ago

@Eric-Arellano Thank you so much for the valuable information. I will keep this in mind. I am learning a lot from you guys. I couldn't be more grateful! Thank You

Shivansh20128 commented 3 days ago

I have created a new PR (and closed the previous one) that contains changes related to only this issue. I apologize for the inconvenience caused. Thank You