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

Fix Sphinx-execution & other minor circuit updates #5269

Merged
merged 16 commits into from
Oct 26, 2020

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Oct 21, 2020

Summary

Blocks qiskit-community/qiskit-aqua#1374.

  • Unblock Aqua CI by using jupyter-execute instead of >>> style examples. The latter is run and tested by Aqua's CI and but hasn't been designed to be valid code in our docs.
  • Remove the 4-months deprecated mirror method
  • Update the existing_gate_names in qasm()
  • Remove the outdated default gates names in draw() and redirect to the DefaultStyle class instead

@Cryoris Cryoris requested a review from a team as a code owner October 21, 2020 08:10
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of small inline comments, also this needs an upgrade release note because you're removing a deprecated method which has upgrade impact for users.

qiskit/circuit/quantumcircuit.py Outdated Show resolved Hide resolved
}

element type in the output visualization.
See :class:`~qiskit.visualization.qcstyle.DefaultStyle` for the defaults.
Copy link
Member

Choose a reason for hiding this comment

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

I don't actually think this is a valid docs link right now. I'm not able to find this path in the documentation https://qiskit.org/documentation/apidoc/visualization.html it probably means we need to add this to the docs first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That just means adding it in the __init__ docstring or do I also need to create a new apidocs file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I referenced the module for now, and added the module to the __init__

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you just need it in the qiskit.visualization __init__ module. There's no need to add a new apidocs.rst unless you want to create a new top page in the docs tree. (which for qcstyle I don't think makes sense, at least not yet until we improve the interface around it).

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 we might need to add some additional setup or docstrings to make this work. Looking at the rendered output from sphinx of this it doesn't contain any real details:

Screenshot_2020-10-21_10-42-21

@Cryoris Cryoris requested a review from mtreinish October 21, 2020 14:59
@enavarro51
Copy link
Contributor

In working on #5223 I was updating the docs for circuit_drawer and QuantumCircuit.draw, which are duplicates except for 1 param each. I talked to @1ucian0 about having the QC.draw docs link to the circuit_drawer docs, but ultimately decided not do it. I like the idea though of referring both to new docs in qcstyle for the style dict, which is where they probably belong.

From what I see here, it looks like you are only moving the color references to qcstyle, not the whole style dict. It seems like we would want the whole style dict documented in qcstyle and then put links to this in QC.draw and circuit_drawer.

@Cryoris
Copy link
Contributor Author

Cryoris commented Oct 22, 2020

@enavarro51 if you're planning to take care of this I'll revert the changes here 👍

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM. I just checked the 0.15.0 release went out on August 6th, so the deprecation minimum is still 11 days away. Normally I'd say lets wait 11 days to merge this, but I think it it's probably pretty unlikely that we'll release before Nov 6th. Since this is blocking aqua I think it's fine to let that slide this time.

@Cryoris Cryoris merged commit fdcae2d into Qiskit:master Oct 26, 2020
@Cryoris Cryoris deleted the minor-circuit-update branch October 26, 2020 12:11
@kdk kdk added the Changelog: None Do not include in changelog label Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants