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

Rename get_matrix/get_eigvals to matrix/eigvals and remove properties with same name #2498

Merged
merged 15 commits into from
May 3, 2022

Conversation

mariaschuld
Copy link
Contributor

Context:

To make the transition from op.matrix to op.matrix() smoother, we used the temporary name get_matrix for the latter for 2 release cycles. The same is true for op.eigvals. This PR removes the property and introduces the final name.

Description of the Change:

  • Removed the two properties
  • Refactored the two functions
  • Removed deprecation tests

As a tangential extra, I also removed the warning that decomposition now is a method, which has been in the code base for a lot of releases now.

Benefits:

Finalises implementations of the operator refactor. Using methods gives us a lot more flexibility (i.e. to have matrix() take wire_order as an argument, and makes explicit that heavy computations may be executed.

Possible Drawbacks:

User code relying on the properties breaks now.

@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 May 2, 2022

Codecov Report

Merging #2498 (54ff26d) into master (bd552ac) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2498      +/-   ##
==========================================
- Coverage   99.48%   99.47%   -0.01%     
==========================================
  Files         243      243              
  Lines       19432    19413      -19     
==========================================
- Hits        19331    19312      -19     
  Misses        101      101              
Impacted Files Coverage Δ
pennylane/gradients/parameter_shift.py 100.00% <ø> (ø)
pennylane/templates/subroutines/qpe.py 100.00% <ø> (ø)
pennylane/transforms/commutation_dag.py 99.69% <ø> (ø)
pennylane/transforms/control.py 100.00% <ø> (ø)
pennylane/transforms/op_transforms.py 100.00% <ø> (ø)
pennylane/_qubit_device.py 98.73% <100.00%> (ø)
pennylane/devices/default_mixed.py 100.00% <100.00%> (ø)
pennylane/devices/default_qubit.py 100.00% <100.00%> (ø)
pennylane/devices/default_qubit_torch.py 92.07% <100.00%> (ø)
pennylane/grouping/utils.py 100.00% <100.00%> (ø)
... and 10 more

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 bd552ac...54ff26d. Read the comment docs.

[ 1 -1]



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 am not sure why these sections show as "added", when only the >>> op.eigvals() line was changed...

@mariaschuld mariaschuld requested a review from josh146 May 2, 2022 11:49
@mariaschuld
Copy link
Contributor Author

I'm not quite sure what codecov is complaining about, since I only renamed and deleted code...

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.

A very nice, straightforward PR to review! Only comment is regarding the decomposition, which should also be mentioned in the changelog

Comment on lines +41 to +43
* The properties `eigval` and `matrix` from the `Operator` class were replaced with the
methods `eigval()` and `matrix(wire_order=None)`.
[(#2498)](https://github.com/PennyLaneAI/pennylane/pull/2498)
Copy link
Member

Choose a reason for hiding this comment

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

👍

pennylane/operation.py Show resolved Hide resolved
@mariaschuld
Copy link
Contributor Author

Codecov seems to complain about having less code, not more uncovered code...will overwrite

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

2 participants