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

[TEMPLATES] Rewrite state preparation templates as operations #1190

Merged
merged 18 commits into from
Apr 8, 2021

Conversation

mariaschuld
Copy link
Contributor

@mariaschuld mariaschuld commented Apr 6, 2021

This is the third PR to refactor the templates (following 1156 and 1163).

Changes:

  • Templates are turned into operations, so that the decomposition is executed in the expand function.
  • The pre-processing was moved into the __init__ function.
  • The tests were moved into a separate file.
  • Docstrings were improved.
  • If the template takes weights, a method to return the shape of the weights was added.

Drawbacks:
The existing MottonenStatePreparation gradient test is insufficient, and it turns out that for most cases gradients do not yet work. However, this is a major task and should be done in a separate PR.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2021

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

.. warning::

Due to non-trivial classical processing of the state vector,
this template is not always fully differentiable.
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 added this warning for now.

@@ -1103,7 +1103,9 @@ def to_openqasm(self, wires=None, rotations=True):
op.inv()

# decompose the queue
operations = tape.expand(stop_at=lambda obj: obj.name in OPENQASM_GATES).operations
operations = tape.expand(
depth=10, stop_at=lambda obj: obj.name in OPENQASM_GATES
Copy link
Contributor Author

@mariaschuld mariaschuld Apr 6, 2021

Choose a reason for hiding this comment

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

We have one more level of "nesting", so need to define depth here.

@@ -1572,7 +1572,7 @@ def test_phase_estimated_two_qubit(self):
)
qml.probs(estimation_wires)

tape = tape.expand()
tape = tape.expand(depth=2)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same problem as above...

@@ -728,7 +729,7 @@ def test_nesting_and_decomposition(self):
qml.probs(wires=0), qml.probs(wires="a")

new_tape = tape.expand()
assert len(new_tape.operations) == 5
assert len(new_tape.operations) == 4
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 thought here we do not have to expand to higher depth, but can just adjust the count...

@@ -1103,7 +1103,9 @@ def to_openqasm(self, wires=None, rotations=True):
op.inv()

# decompose the queue
operations = tape.expand(stop_at=lambda obj: obj.name in OPENQASM_GATES).operations
operations = tape.expand(
depth=2, stop_at=lambda obj: obj.name in OPENQASM_GATES
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have one more level of nesting, so need to increase the depth here.

@codecov
Copy link

codecov bot commented Apr 6, 2021

Codecov Report

Merging #1190 (62c6b43) into master (fd99be7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1190   +/-   ##
=======================================
  Coverage   98.12%   98.12%           
=======================================
  Files         145      145           
  Lines       10893    10903   +10     
=======================================
+ Hits        10689    10699   +10     
  Misses        204      204           
Impacted Files Coverage Δ
pennylane/tape/tape.py 98.51% <100.00%> (ø)
.../state_preparations/arbitrary_state_preparation.py 100.00% <100.00%> (ø)
pennylane/templates/state_preparations/basis.py 100.00% <100.00%> (ø)
pennylane/templates/state_preparations/mottonen.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 fd99be7...62c6b43. Read the comment docs.

Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

Do we need to catch if state preparation is not occurring at the beginning of a circuit on a 00000... state?

I tested

@qml.qnode(dev)
def circuit():
    qml.PauliX(wires=(0))
    qml.templates.state_preparations.basis.BasisStatePreparation([1,0,0], wires=(0,1,2))
    return qml.state()

and that lead to non-intuitive behaviour.

I left a lot of comments, but most of them are just suggestions.

The things that should really be changed are:

  • placing test_basis_state_prep.TestInputs.test_exception_wrong_dim in the correct file
  • using pytest.importorskip in interface tests
  • docstring for ArbitraryStatePreparation

assert shape == expected_shape


def circuit_template(weights):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be fixtures? Just curious.

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 think it's ok if they are just functions defined somewhere...since they change in every test, so it would be no use to make them global fixtures...Or did I understand wrong?

I am never sure if I should put them into the test class, but I find it so ugly to refer to them with self...

pennylane/templates/state_preparations/mottonen.py Outdated Show resolved Hide resolved
@mariaschuld
Copy link
Contributor Author

Thanks @albi3ro for your great review!

The bug about BasisStatePreparation not being checked for being the first operation in a circuit is indeed confusing. Can we solve this in a separate issue though, since it would be a breaking change? I will open one!

@mariaschuld mariaschuld requested a review from albi3ro April 7, 2021 07:28
Copy link
Contributor

@albi3ro albi3ro 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!

It was great to learn about a new section of code and thanks for answering all my questions and nitpicks 👍

@mariaschuld mariaschuld merged commit 8480ab7 into master Apr 8, 2021
@mariaschuld mariaschuld deleted the rewrite-state-preps branch April 8, 2021 06:37
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.

2 participants