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

Collect expand_fns and stopping criteria into central file #1734

Merged
merged 84 commits into from
Oct 17, 2021

Conversation

dwierichs
Copy link
Contributor

Context:
tape expansion functions are scattered across PL, this PR collects them, introducing a handy wrapping class that enables logical conjunction of callables.

Description of the Change:
Introduce StoppingCriterion, a new wrapper class, as well as a convenience function get_expand_fn to create an expand_fn from a stopping criterion.

Benefits:
Centralized expansion functions and convenient composition of stopping criteria.

Possible Drawbacks:
Likely a bit less performant that explicitly writing out the logical functions locally in the corresponding files.

Related GitHub Issues:
#1732

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.

@dwierichs 🎉 The new naming and structure now feels 💯!

All my comments and suggestions are minor, and mainly wrt documentation 🙂

pennylane/boolean_fn.py Outdated Show resolved Hide resolved
pennylane/boolean_fn.py Outdated Show resolved Hide resolved
pennylane/boolean_fn.py Show resolved Hide resolved
pennylane/boolean_fn.py Outdated Show resolved Hide resolved
pennylane/boolean_fn.py Outdated Show resolved Hide resolved
pennylane/transforms/tape_expand.py Outdated Show resolved Hide resolved
tests/fourier/test_qnode_spectrum.py Outdated Show resolved Hide resolved
tests/test_operation.py Show resolved Hide resolved
tests/transforms/test_metric_tensor.py Outdated Show resolved Hide resolved
doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
@dwierichs
Copy link
Contributor Author

dwierichs commented Oct 15, 2021

Hi @josh146 I think everything should be addressed now :)

  1. I now went for create_expand_fn instead of generate_expand_fn, because it is shorter and "creating" feels closer to function construction than "generating".
  2. Should the BooleanFn qml.operation.has_unitary_gen and the flag RX.has_unitary_generator have the same name? I don't think so, to keep them distinct, but I'm not entirely sure (in particular because the former only queries the latter).
  3. I like the wrapping approach for BooleanFn and went for that :)

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.

This is such a nice addition @dwierichs 😍

Happy to approve, conditional on my suggestions below. Don't forget to add the tape_expand functions to the transforms/__init__.py module docstring, and check that they render correctly in the PR documentation build!

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
pennylane/operation.py Show resolved Hide resolved
Comment on lines +1927 to +1930
@qml.BooleanFn
def is_measurement(obj):
"""Returns ``True`` if an operator is a ``MeasurementProcess`` instance."""
return isinstance(obj, qml.measure.MeasurementProcess)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very nice that you caught this one! :)

pennylane/transforms/tape_expand.py Show resolved Hide resolved
pennylane/transforms/tape_expand.py Show resolved Hide resolved
tests/transforms/test_tape_expand.py Outdated Show resolved Hide resolved
tests/transforms/test_tape_expand.py Outdated Show resolved Hide resolved
tests/transforms/test_tape_expand.py Outdated Show resolved Hide resolved
tests/transforms/test_tape_expand.py Outdated Show resolved Hide resolved
@dwierichs dwierichs merged commit 31c2430 into master Oct 17, 2021
@dwierichs dwierichs deleted the collect-expand_fns branch October 17, 2021 09:34
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