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

Improve ZZFeatureMap API docs #12431

Closed
Eric-Arellano opened this issue May 17, 2024 · 12 comments · Fixed by #13231
Closed

Improve ZZFeatureMap API docs #12431

Eric-Arellano opened this issue May 17, 2024 · 12 comments · Fixed by #13231
Assignees
Labels
documentation Something is not clear or an error documentation

Comments

@Eric-Arellano
Copy link
Collaborator

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
@Eric-Arellano Eric-Arellano added the documentation Something is not clear or an error documentation label May 17, 2024
@nonhermitian
Copy link
Contributor

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

@Shivansh20128
Copy link
Contributor

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

@Shivansh20128
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

Shivansh20128 commented Sep 25, 2024

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
Copy link
Contributor

@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
Copy link
Collaborator Author

Eric-Arellano commented Sep 25, 2024

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
Copy link
Contributor

Cryoris commented Sep 25, 2024

Some comments and questions:

  • The docstring code examples are indeed already fixed on main 🙂
  • The attributes num_parameters, parameters and data are coming from the BlueprintCircuit, which inherits from QuantumCircuit. If the docstring is not overriden, is the docstring not pulled from the parent object (i.e. QuantumCircuit)?
  • The same holds for entanglement_blocks (ZZFeatureMap should show the docstring of the parent NLocal)

@Shivansh20128
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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

@Eric-Arellano
Copy link
Collaborator Author

You can see in Qiskit/documentation#2071 that #13231 indeed fixed the problems in the original post 🚀 Thank you @Shivansh20128 for making Qiskit better for everyone!

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants