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

doc: Add matrix expressions to docstrings #756

Merged
merged 14 commits into from
Nov 16, 2023
Merged

Conversation

ykharkov
Copy link
Member

@ykharkov ykharkov commented Oct 23, 2023

Resolves issue #597

Description of changes:
Added definitions of unitary matrices for each gate (docstrings)

Testing done:

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

Tests

  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ykharkov ykharkov requested a review from a team as a code owner October 23, 2023 15:34
@@ -109,6 +109,12 @@ def h(
Returns:
Iterable[Instruction]: `Iterable` of H instructions.

Unitary matrix:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might work if you put it below the examples section.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @krneta. I decided to move it to the top instead, so that it shows the matrix immediately.

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f1a549a) 100.00% compared to head (a98a40e) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #756   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          129       129           
  Lines         8374      8374           
  Branches      1866      1866           
=========================================
  Hits          8374      8374           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@rmshaffer rmshaffer left a comment

Choose a reason for hiding this comment

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

This is looking awesome. For reference, here is an example of the rendered doc for this PR:
https://amazon-braket-sdk-python--756.org.readthedocs.build/en/756/_apidoc/braket.circuits.gates.html#braket.circuits.gates.CPhaseShift00

General question, do you have any idea how hard it would be to have the same docstring appear in the circuit documentation for these gates?
https://amazon-braket-sdk-python--756.org.readthedocs.build/en/756/_apidoc/braket.circuits.circuit.html#braket.circuits.circuit.Circuit.cphaseshift00
or maybe somehow have the circuit API doc link back to the gates API doc? I'm not sure which page users are more likely to land on, but it would be nice if both lead them to this matrix definition somehow.

src/braket/circuits/gates.py Outdated Show resolved Hide resolved
src/braket/circuits/gates.py Outdated Show resolved Hide resolved
src/braket/circuits/gates.py Outdated Show resolved Hide resolved
src/braket/circuits/gates.py Outdated Show resolved Hide resolved
@AbeCoull
Copy link
Contributor

To @rmshaffer's point, we can add this within the subroutine docstrings. This should render on the circuit page but this is something to double check with a single gate first.

@ykharkov
Copy link
Member Author

ykharkov commented Nov 1, 2023

@math411
I think both me and @rmshaffer did not fully understand your suggestion regarding this comment:

General question, do you have any idea how hard it would be to have the same docstring appear in the circuit documentation for these gates?

we can add this within the subroutine docstrings. This should render on the circuit page but this is something to double check with a single gate first.

Are we OK merging as it is? Otherwise please advise on the next steps.

@ajberdy
Copy link
Contributor

ajberdy commented Nov 2, 2023

We will remove the generic line about "registering the subroutine" from the .h etc methods of the gates and instead insert the class documentation (with the gate name, matrix etc)

@AbeCoull
Copy link
Contributor

AbeCoull commented Nov 2, 2023

@math411 I think both me and @rmshaffer did not fully understand your suggestion regarding this comment:

General question, do you have any idea how hard it would be to have the same docstring appear in the circuit documentation for these gates?

we can add this within the subroutine docstrings. This should render on the circuit page but this is something to double check with a single gate first.

Are we OK merging as it is? Otherwise please advise on the next steps.

@ykharkov my idea was, as an example, change L101 to:

        r"""Registers this function into the circuit class.
        
        Unitary matrix:

                .. math:: \mathtt{H} = \frac{1}{\sqrt{2}} \begin{bmatrix}
                        1 & 1 \\
                        1 & -1 \end{bmatrix}.
    """

@rmshaffer
Copy link
Contributor

rmshaffer commented Nov 3, 2023

We will remove the generic line about "registering the subroutine" from the .h etc methods of the gates and instead insert the class documentation (with the gate name, matrix etc)

@ajberdy could you clarify the "we" you are referring to? Should the change you're suggesting be made as part of this PR, or is that going to happen separately?

@ajberdy ajberdy mentioned this pull request Nov 14, 2023
5 tasks
@ajberdy
Copy link
Contributor

ajberdy commented Nov 14, 2023

We will remove the generic line about "registering the subroutine" from the .h etc methods of the gates and instead insert the class documentation (with the gate name, matrix etc)

@ajberdy could you clarify the "we" you are referring to? Should the change you're suggesting be made as part of this PR, or is that going to happen separately?

@math411 has updated the PR to include the matrices in the circuit subroutine method. I recommend the PR is updated to replace "Registers this function into the circuit class." with the description of the gate for the class doc string, i.e. "Pauli-X gate."

@AbeCoull
Copy link
Contributor

AbeCoull commented Nov 14, 2023

Added in #800 to have a more programmatic way of doing this.

Accidentally pushed those changes to this branch, I can roll back those changes in favor of this approach (or a more clean version of this).

@ykharkov
Copy link
Member Author

Added in #800 to have a more programmatic way of doing this.

Accidentally pushed those changes to this branch, I can roll back those changes in favor of this approach (or a more clean version of this).

Thanks @math411 and @ajberdy .

I addressed concern of @ajberdy about "Registers this function into the circuit class." line and replaced with an appropriate gate description.

Let's merge this PR to keep the ball moving and in the followup PR from @math411 we can add a more programatic solution.

Thanks

Copy link
Contributor

@ajberdy ajberdy left a comment

Choose a reason for hiding this comment

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

Capturing that we will have to remove the "Args" section from the class doc strings as that is actually broken right now (doc link). Though it is broken before and after this PR, so not going to block.

@AbeCoull AbeCoull changed the title Add matrix expressions to docstrings doc: Add matrix expressions to docstrings Nov 15, 2023
@ykharkov ykharkov merged commit 0485562 into main Nov 16, 2023
23 checks passed
@ykharkov ykharkov deleted the feature/gate_docstrings branch November 16, 2023 16:02
guomanmin pushed a commit that referenced this pull request Dec 4, 2023
* Add matrix expressions to docstrings

* linter

* fix r-strings

* Capitalize H gate

* Rename theta->phi

* Address comments

* Update src/braket/circuits/gates.py

Co-authored-by: Ryan Shaffer <[email protected]>

* add matrices to subrountines

* Remove [Registers this function into the circuit class] line from docstrings

---------

Co-authored-by: Abe Coull <[email protected]>
Co-authored-by: Cody Wang <[email protected]>
Co-authored-by: Ryan Shaffer <[email protected]>
Co-authored-by: Abe Coull <[email protected]>
Co-authored-by: Aaron Berdy <[email protected]>
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.

6 participants