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 embeddings as operations #1156

Merged
merged 92 commits into from
Apr 6, 2021
Merged

Conversation

mariaschuld
Copy link
Contributor

@mariaschuld mariaschuld commented Mar 22, 2021

This is part of making all templates inherit from Operations.

In the process, the tests were mostly rewritten from scratch, the docstrings improved and the dependency on the broadcast function was removed for reasons of speed. Otherwise, code was just moved.

It is quite hard to review this PR. I commented on everything that differs from the previous template design, and it would be great if the review could focus on the tests.

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @mariaschuld! I still have some tests to look through, but here are some initial comments.

.github/CHANGELOG.md Show resolved Hide resolved
pennylane/templates/embeddings/amplitude.py Show resolved Hide resolved
pennylane/templates/embeddings/amplitude.py Show resolved Hide resolved
pennylane/templates/embeddings/angle.py Show resolved Hide resolved
pennylane/templates/embeddings/basis.py Outdated Show resolved Hide resolved
pennylane/templates/embeddings/basis.py Show resolved Hide resolved
@trbromley trbromley self-requested a review March 26, 2021 15:53

@template
def IQPEmbedding(features, wires, n_repeats=1, pattern=None):
class IQPEmbedding(Operation):
r"""
Encodes :math:`n` features into :math:`n` qubits using diagonal gates of an IQP circuit.
Copy link
Contributor

Choose a reason for hiding this comment

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

what does "IQP" stand for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The embedding encodes into the diagonal gates of an IQP circuit...once more a case of an embedding which does not (yet) have a good name in the community

Copy link
Contributor

@agran2018 agran2018 left a comment

Choose a reason for hiding this comment

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

Thanks @mariaschuld for showing us how this is done. Now, that I have got the general idea, will give it a second round in the afternoon. I have noticed that codefactor is complaining in different places.

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @mariaschuld! I have left some comments and minor suggestions, but overall approve 💯

tests/templates/test_embeddings/test_angle.py Outdated Show resolved Hide resolved
tests/templates/test_embeddings/test_angle.py Show resolved Hide resolved
tests/templates/test_embeddings/test_basis.py Outdated Show resolved Hide resolved
tests/templates/test_embeddings/test_displacement_emb.py Outdated Show resolved Hide resolved
tests/templates/test_embeddings/test_displacement_emb.py Outdated Show resolved Hide resolved
tests/templates/test_embeddings/test_iqp_emb.py Outdated Show resolved Hide resolved
tests/templates/test_embeddings/test_squeezing_emb.py Outdated Show resolved Hide resolved
@mariaschuld
Copy link
Contributor Author

@agran2018 can I merge? :)

Copy link
Contributor

@agran2018 agran2018 left a comment

Choose a reason for hiding this comment

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

Thanks @mariaschuld. I noticed that there is still an unrelated warning being issue by CodeFactor at the end.

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

5 participants