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

Add grouping module to docs #850

Merged
merged 41 commits into from
Oct 19, 2020
Merged

Add grouping module to docs #850

merged 41 commits into from
Oct 19, 2020

Conversation

antalszava
Copy link
Contributor

@antalszava antalszava commented Oct 14, 2020

Description of the Change:
Adds a description of the grouping subpackage to the Quantum operations page in the user-facing quickstart docs:

  • Added doc/code/qml_grouping.rst for subpackage description with modules
  • Added a short description and an example to doc/introduction/operations.rst
  • Renamed certain functions

Note: placed more mechanic updates of docstrings into #852 for ease of reviewing. That PR targets the branch of this PR. Therefore it can be worth considering that PR first.

@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #850 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #850   +/-   ##
=======================================
  Coverage   97.79%   97.79%           
=======================================
  Files         136      137    +1     
  Lines        9201     9207    +6     
=======================================
+ Hits         8998     9004    +6     
  Misses        203      203           
Impacted Files Coverage Δ
pennylane/grouping/graph_colouring.py 100.00% <ø> (ø)
pennylane/grouping/__init__.py 100.00% <100.00%> (ø)
pennylane/grouping/group_observables.py 100.00% <100.00%> (ø)
pennylane/grouping/optimize_measurements.py 92.30% <100.00%> (ø)
pennylane/grouping/transformations.py 100.00% <100.00%> (ø)
pennylane/grouping/utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0119620...5ee57cb. Read the comment docs.

@@ -109,6 +109,7 @@ h1 {
/*border-bottom: 3px solid #D8E4EF;*/
/*border-bottom: 3px solid #eee;*/
padding-bottom: -29px;
word-wrap: break-word;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is meant to wrap the long method/function names rendered in the docs

@@ -17,10 +17,10 @@

from pennylane.wires import Wires
from pennylane.grouping.utils import (
convert_observables_to_binary_matrix,
observables_to_binary_matrix,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sumarry of the renaming:
- diagonalize_qwc_grouping -> diagonalize_qwc_pauli_words
- obtain_qwc_post_rotations_and_diagonalized_groupings -> diagonalize_qwc_groupings
- convert_observables_to_binary_matrix -> observables_to_binary_matrix
- get_qwc_complement_adj_matrix -> qwc_complement_adj_matrix

Copy link
Member

@josh146 josh146 Oct 16, 2020

Choose a reason for hiding this comment

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

Thanks @antalszava!

@co9olguy, for context as to why these were renamed: they include redundant terms (get, obtain, convert), and are too long - they were causing excessive horizontal scrolling and word wrapping in the documentation. The latter point wasn't evident until this PR, when they were added to the API documentation.

In particular, obtain_qwc_post_rotations_and_diagonalized_groupings was not displaying well in the documentation at all!

@antalszava antalszava marked this pull request as ready for review October 16, 2020 01:10
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Thanks @antalszava! Since some of the functions are renamed, could you also update the changelog?

doc/code/qml_grouping.rst Outdated Show resolved Hide resolved
Comment on lines 140 to 161
Grouping Pauli words
^^^^^^^^^^^^^^^^^^^^

Grouping Pauli words can be used for the optimizing the measurement of qubit
Hamiltonians. Along with groups of observables, post measurement rotations can
also be obtained using :func:`~.optimize_measurements`:

.. code-block:: python

>>> obs = [qml.PauliY(0), qml.PauliX(0) @ qml.PauliX(1), qml.PauliZ(1)]
>>> coeffs = [1.43, 4.21, 0.97]
>>> post_rotations, diagonalized_groupings, grouped_coeffs = optimize_measurements(obs, coeffs)
>>> post_rotations
[[RY(-1.5707963267948966, wires=[0]), RY(-1.5707963267948966, wires=[1])],
[RX(1.5707963267948966, wires=[0])]]

The post measurement rotations can be used to diagonalize the partitions of
observables found.

For further details on measurement optimization, graph coloring to be solved
for grouping observables and auxiliary functions refer to the
:doc:`/code/qml_grouping` subpackage.
Copy link
Member

Choose a reason for hiding this comment

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

👍 This looks great @antalszava. HIghlights that this functionality exists, but keeps it short, sweet, and succinct, which is how a quickstart should read (the quickstarts are reference guides, first and foremost, not tutorials).

If a user would like more information on the grouping functions, they can then refer to the qml.grouping module page.

doc/introduction/operations.rst Outdated Show resolved Hide resolved
pennylane/grouping/group_observables.py Outdated Show resolved Hide resolved
doc/code/qml_grouping.rst Outdated Show resolved Hide resolved
doc/code/qml_grouping.rst Outdated Show resolved Hide resolved
doc/code/qml_grouping.rst Outdated Show resolved Hide resolved
doc/code/qml_grouping.rst Outdated Show resolved Hide resolved
Comment on lines 58 to 60
ValueError: if arguments specified for `grouping_type` or
`graph_colourer` are not recognized as elements of `GROUPING_TYPES` or
`GRAPH_COLOURING_METHODS` respectively
Copy link
Member

Choose a reason for hiding this comment

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

Note that GROUPING_TYPES and GRAPH_COLOURING_METHODS are not displayed in the documentation, so the user reading this will not know what the elements are. Perhaps they should be duplicated in the description of grouping_type and graph_colourer.

@@ -107,15 +107,15 @@ def diagonalize_pauli_word(pauli_word):
return diag_term


def diagonalize_qwc_grouping(qwc_grouping):
def diagonalize_qwc_pauli_words(qwc_grouping):
Copy link
Member

Choose a reason for hiding this comment

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

👍 This definitely makes more sense after reading the first line of the docstring.

antalszava and others added 8 commits October 16, 2020 13:01
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
pennylane/grouping/optimize_measurements.py Outdated Show resolved Hide resolved
pennylane/grouping/__init__.py Outdated Show resolved Hide resolved
doc/introduction/operations.rst Outdated Show resolved Hide resolved
doc/introduction/operations.rst Outdated Show resolved Hide resolved
Comment on lines 159 to 160
For further details on measurement optimization, graph coloring to be solved
for grouping observables, and auxiliary functions, refer to the
Copy link
Member

Choose a reason for hiding this comment

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

The phrase "graph coloring to be solved for grouping observables" is a bit awkward (I wasn't sure what it meant). Suggest rephrasing (and using the Canadian spelling "colouring")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Rephrased this part as For further details on measurement optimization, the minimum clique cover problem whose solution is used for grouping observables.

Copy link
Member

Choose a reason for hiding this comment

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

🤔 still seems to be a verb or something missing from the new phrasing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had another go with it, let me know if it would be nice to further rephrase

doc/code/qml_grouping.rst Outdated Show resolved Hide resolved
doc/code/qml_grouping.rst Outdated Show resolved Hide resolved
doc/code/qml_grouping.rst Outdated Show resolved Hide resolved
doc/code/qml_grouping.rst Outdated Show resolved Hide resolved
doc/code/qml_grouping.rst Outdated Show resolved Hide resolved
@antalszava
Copy link
Contributor Author

@josh146 @co9olguy thank you so much for the many great suggestions! 🎊 Addressed them (maybe had one question for a minor suggestion).

@@ -86,7 +90,7 @@ def __init__(self, observables, grouping_type="qwc", graph_colourer="rlf"):
self.adj_matrix = None
self.grouped_paulis = None

def obtain_binary_repr(self, n_qubits=None, wire_map=None):
def binary_repr(self, n_qubits=None, wire_map=None):
Copy link
Contributor Author

@antalszava antalszava Oct 16, 2020

Choose a reason for hiding this comment

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

Removed obtain from the method names here for brevity

antalszava and others added 5 commits October 16, 2020 17:53
Co-authored-by: Nathan Killoran <co9olguy@users.noreply.github.com>
Co-authored-by: Nathan Killoran <co9olguy@users.noreply.github.com>
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Looks great now @antalszava.

Note: I realized that the namespacing was a bit excessive (e.g., we didn't need qml.grouping.group_observables.group_observables). I fixed this in 42b4ba5 by importing all functions directly into grouping/__init__.py (with the exception of the graph_colouring functions, since the functions names are not descriptive enough).

Let me know what you think - if it looks okay, feel free to merge this in (as well as #852)

antalszava and others added 4 commits October 18, 2020 15:24
* New placements and renaming

* Shorten names in example

* Shorten names in example

* Shorten names in example

* Returns section indentation; no new line at the end of docstrings

* Returns, raises formatting

* Double back ticks

* String options with double backticks

* Double backticks in tests

* Keyword args tidy

* array[bool]->array[int]

* scalar->float

* Update pennylane/grouping/optimize_measurements.py

Co-authored-by: Josh Izaac <josh146@gmail.com>

* Indentation, double backticks

* Minor updates

* Class refs

* Indentation

* Update pennylane/grouping/graph_colouring.py

Co-authored-by: Nathan Killoran <co9olguy@users.noreply.github.com>

* Suggestion from code review

Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Nathan Killoran <co9olguy@users.noreply.github.com>
@josh146 josh146 merged commit 9d21ea4 into master Oct 19, 2020
@josh146 josh146 deleted the grouping_in_docs branch October 19, 2020 05:45
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.

None yet

3 participants