Skip to content

Explanation and examples added for various QuantumCircuit methods Fixes #3400#3683

Merged
kdk merged 50 commits into
Qiskit:masterfrom
sumitpuri:fixissue3640branch
Feb 6, 2020
Merged

Explanation and examples added for various QuantumCircuit methods Fixes #3400#3683
kdk merged 50 commits into
Qiskit:masterfrom
sumitpuri:fixissue3640branch

Conversation

@sumitpuri
Copy link
Copy Markdown
Contributor

Summary

Fixes #3400 by adding explanations for various QuantumCircuit methods.

Details and comments

The gates (ccx, ch, crz, cswap, cu1, cu3, cx, cy, cz, h, iden, rx, ry, rz, s, sdg, swap, t, tdg, u1, u2, u3, x, y, z) now have an example and explanation of what the respective gate does.

1ucian0 and others added 12 commits January 3, 2020 15:22
Co-Author: Sumit Puri <er.sumitpuri@gmail.com>
Examples have been provided for the following methods- ccx, ch, crz, cswap, cu1, cu3, cx, cy, cz, h, iden, rx, ry, rz, s, sdg, swap, t, tdg, u1, u2, u3, x, y, z
This reverts commit 36b7881.
Examples have been provided for the following methods- ccx, ch, crz, cswap, cu1, cu3, cx, cy, cz, h, iden, rx, ry, rz, s, sdg, swap, t, tdg, u1, u2, u3, x, y, z
@nonhermitian
Copy link
Copy Markdown
Contributor

The examples should be in jupyter-execute blocks so as they are run at docs build time. This way errors in the docs are caught.

@sumitpuri
Copy link
Copy Markdown
Contributor Author

@nonhermitian So are you suggesting to split the explanation and the example circuit? Are the one line explanations okay in these files? Can you please share a sample screenshot of where the examples need to be added?

@sumitpuri
Copy link
Copy Markdown
Contributor Author

I was doing a PR for another bug #3684 but seems like it got tied to this PR instead.

@kdk
Copy link
Copy Markdown
Member

kdk commented Jan 7, 2020

The explanations and examples can stay together in the same docstrings, but the example should be prefaced with .. jupyter-execute:: so that they will be run and displayed in the auto-generated documentation. For example, https://github.com/Qiskit/qiskit-terra/blob/7718e3576d0c9d77991c8c4b813ac05b7b1aa940/qiskit/circuit/quantumcircuit.py#L76
builds to the Examples section on https://qiskit.org/documentation/api/qiskit.circuit.QuantumCircuit.html#quantumcircuit .

@kdk
Copy link
Copy Markdown
Member

kdk commented Jan 7, 2020

I was doing a PR for another bug #3684 but seems like it got tied to this PR instead.

You can revert that commit from this PR by checking out the fixissue3640branch branch and running git revert b8b010e.

@sumitpuri
Copy link
Copy Markdown
Contributor Author

Thank you @kdk Can you help me fix the two checks that are failing.

Copy link
Copy Markdown
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

It looks like a missing import statement. This should fix it.

Comment thread qiskit/extensions/standard/crz.py Outdated
@kdk
Copy link
Copy Markdown
Member

kdk commented Feb 3, 2020

@kdk I have incorporated the comments, the tests are still failing though.

#3631 moved the definitions of some of the controlled gate classes into the files of their base gates (e.g. cx and ccx are now defined in x.py). Normally changes on master are automatically merged in to PR branches, but that change will have to be resolved manually.

@sumitpuri
Copy link
Copy Markdown
Contributor Author

ok @kdk so what is the suggested action item for me?

Copy link
Copy Markdown
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

ok @kdk so what is the suggested action item for me?

I resolved the conflicts so no action needed from you :). One comment below on the matrix definition of cswap.

Comment thread qiskit/extensions/standard/swap.py Outdated
Fixed (#3400)

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>
@sumitpuri
Copy link
Copy Markdown
Contributor Author

Thanks @kdk However, it is failing here:
But I don't see anything wrong in the code.

image

Copy link
Copy Markdown
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

A few more import locations need updating.

Comment thread qiskit/extensions/standard/h.py Outdated
Comment thread qiskit/extensions/standard/x.py
Comment thread qiskit/extensions/standard/z.py
Comment thread qiskit/extensions/standard/swap.py Outdated
Comment thread qiskit/extensions/standard/x.py Outdated
@kdk kdk added this to the 0.12 milestone Feb 4, 2020
sumitpuri and others added 3 commits February 4, 2020 09:23
Fixed (#3400)

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>
Fixed (#3400)

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>
Fixed (#3400)

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>
@sumitpuri
Copy link
Copy Markdown
Contributor Author

Thanks @kdk all tests have passed this time.

Copy link
Copy Markdown
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

This looks just about ready to go. to_matrix was re-introduced on RZGate (maybe in one of the merge conflicts) and should be removed.

@sumitpuri
Copy link
Copy Markdown
Contributor Author

Hi @kdk

In rz.py, there's just a to_matrix definition, not the .. jupyter-execute::

Are you suggesting to remove def to_matrix(self) code?

image

@kdk
Copy link
Copy Markdown
Member

kdk commented Feb 6, 2020

Are you suggesting to remove def to_matrix(self) code?

Yes, it had caused a test failure earlier due to a phase difference between Rz and U1 (not sure why the test currently passes) #3683 (comment)

There are some additional merge conflicts, I'll take care of removing it there.

Copy link
Copy Markdown
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

This looks good to go, thanks @sumitpuri !

@kdk kdk merged commit 1de26bb into Qiskit:master Feb 6, 2020
faisaldebouni pushed a commit to faisaldebouni/qiskit-terra that referenced this pull request Aug 5, 2020
Qiskit#3400 (Qiskit#3683)

* Fixed issue 3640 by adding logic for complex imaginary numbers

Co-Author: Sumit Puri <er.sumitpuri@gmail.com>

* Added test for display of 'pi'.

* Added test for displaying 'pi' for both real and imaginary parts of complex number.

* Release notes added for Fixes Qiskit#3640

* release notes

* Fixes Qiskit#3400

Examples have been provided for the following methods- ccx, ch, crz, cswap, cu1, cu3, cx, cy, cz, h, iden, rx, ry, rz, s, sdg, swap, t, tdg, u1, u2, u3, x, y, z

* Revert "Fixes Qiskit#3400"

This reverts commit 36b7881.

* Fixes Qiskit#3400

Examples have been provided for the following methods- ccx, ch, crz, cswap, cu1, cu3, cx, cy, cz, h, iden, rx, ry, rz, s, sdg, swap, t, tdg, u1, u2, u3, x, y, z

* Added release notes for Fixes Qiskit#3400

* Fixes Qiskit#3684 Text drawer displays cU1 correctly.

* Revert "Fixes Qiskit#3684 Text drawer displays cU1 correctly."

This reverts commit b8b010e.

* Fixes Qiskit#3400 by adding jupyter-execute statement in the examples.

* Fixes Qiskit#3400 by adding jupyter-execute statement in the examples and correcting by performing 'make style'.

* Update qiskit/extensions/standard/crz.py

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Revert "Update qiskit/extensions/standard/crz.py"

This reverts commit 2abc824.

* Fixes Qiskit#3400 Added import numpy

* Qiskit#3400 added import numpy statements

* Fixes Qiskit#3400 incorporated comments from @nonhermitian

* Fixes Qiskit#3400 added more text.

* Update qiskit/extensions/standard/rx.py

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Added params for matrix generation.

* Removed Matrix representation from rz.py

* Update qiskit/extensions/standard/z.py

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Update qiskit/extensions/standard/y.py

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Update qiskit/extensions/standard/y.py

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Update qiskit/extensions/standard/u2.py

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Update qiskit/extensions/standard/cx.py

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Update qiskit/extensions/standard/u3.py

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Fixes Qiskit#3400 incorporated suggestions

* Fixes Qiskit#3400 H gate description.

* Added matrix for cswap

* Consistency with removing hyphen.

* Fixes Qiskit#3400 consistency

* Fixing style

* Rearranged the import order

* Fixes matrix representation

* Incorporating suggestions

* Incorporated comments (Qiskit#3400)

* Update qiskit/extensions/standard/swap.py

Fixed (Qiskit#3400)

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Update qiskit/extensions/standard/h.py

Fixed (Qiskit#3400)

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Update qiskit/extensions/standard/swap.py

Fixed (Qiskit#3400)

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Update qiskit/extensions/standard/x.py

Fixed (Qiskit#3400)

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Fix lint failures following merge resolution.

Co-authored-by: Luciano Bello <luciano.bello@ibm.com>
Co-authored-by: Kevin Krsulich <kevin@krsulich.net>
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.

QuantumCircuit methods need examples and better documentation

4 participants