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 CtrlSequence operator #4688

Closed
wants to merge 17 commits into from
Closed

Add CtrlSequence operator #4688

wants to merge 17 commits into from

Conversation

lillian542
Copy link
Contributor

@lillian542 lillian542 commented Oct 17, 2023

Adding a ControlledSequence template as part of the Modular QPE project

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.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.

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aa626e3) 99.64% compared to head (30ca228) 99.63%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4688      +/-   ##
==========================================
- Coverage   99.64%   99.63%   -0.01%     
==========================================
  Files         378      379       +1     
  Lines       34071    33859     -212     
==========================================
- Hits        33950    33737     -213     
- Misses        121      122       +1     
Files Coverage Δ
pennylane/operation.py 97.27% <100.00%> (-0.02%) ⬇️
pennylane/ops/op_math/composite.py 100.00% <100.00%> (ø)
pennylane/ops/op_math/controlled.py 99.61% <ø> (-0.01%) ⬇️
pennylane/ops/op_math/pow.py 100.00% <ø> (ø)
pennylane/ops/op_math/symbolicop.py 100.00% <100.00%> (ø)
pennylane/templates/subroutines/__init__.py 100.00% <100.00%> (ø)
...ylane/templates/subroutines/controlled_sequence.py 100.00% <100.00%> (ø)

... and 41 files with indirect coverage changes

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

Copy link
Contributor

@KetpuntoG KetpuntoG left a comment

Choose a reason for hiding this comment

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

Thank you Lillian! I have been testing and the results are correct. However, I have tried testing the gradient and for some reason it doesn't work. Do you know why it could be?

pennylane/templates/subroutines/ctrlsequence.py Outdated Show resolved Hide resolved
pennylane/templates/subroutines/ctrlsequence.py Outdated Show resolved Hide resolved
pennylane/templates/subroutines/ctrlsequence.py Outdated Show resolved Hide resolved
pennylane/templates/subroutines/__init__.py Outdated Show resolved Hide resolved
@trbromley trbromley added this to the v0.33 milestone Oct 18, 2023
@lillian542
Copy link
Contributor Author

Thank you Lillian! I have been testing and the results are correct. However, I have tried testing the gradient and for some reason it doesn't work. Do you know why it could be?

I looked at it a bit yesterday afternoon and I'm currently not sure, but working on it. I'll mention it in the core stand-up today. For the autograd gradient, its going in as a pnp.array and coming out as an ArrayBox, which I know I've seen before, but I can't remember what the problem was.

@trbromley
Copy link
Contributor

@lillian542 please could you link to the story?

@lillian542
Copy link
Contributor Author

[sc-45846]

@lillian542 lillian542 marked this pull request as ready for review October 19, 2023 21:33
Copy link
Contributor

@soranjh soranjh left a comment

Choose a reason for hiding this comment

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

Thanks @lillian542 and @KetpuntoG! Left some comments and questions. Will approve when they are addressed.

doc/_static/templates/subroutines/big_ctrl.png Outdated Show resolved Hide resolved
doc/_static/templates/subroutines/small_ctrl.png Outdated Show resolved Hide resolved
pennylane/operation.py Show resolved Hide resolved
Copy link
Contributor

@KetpuntoG KetpuntoG left a comment

Choose a reason for hiding this comment

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

A lot of work here! Thank you very much for the effort, @lillian542 🚀

Comment on lines 331 to 334
# pylint: disable=missing-function-docstring
@property
def basis(self):
return self.base.basis
Copy link
Contributor

Choose a reason for hiding this comment

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

This inherits from symbolic op so you don't need this since it will be inherited anyways

Copy link
Contributor

Choose a reason for hiding this comment

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

Operator versus operation thing. easier just to push the attribute up to operator.

Copy link
Contributor

Choose a reason for hiding this comment

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

this can be removed, it is indeed the same as its super()

@mlxd mlxd closed this Oct 23, 2023
@mlxd mlxd deleted the ctrlsequence branch October 23, 2023 15:11
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.

8 participants