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

Making wires the last positional argument for several classes #5643

Closed
wants to merge 14 commits into from

Conversation

PietropaoloFrisoni
Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni commented May 3, 2024

Context: Currently, as noticed in this issue, qml.GateFabric, qml.AllSinglesDoubles, and qml.BasisRotation do not have wires as the last positional argument, which is a requirement for pennylane operations. This might be confusing for users.

Description of the Change: In qml.GateFabric, we made init_state a kwarg instead of a positional argument, with an initial default state as the all-zeros state. For qml.AllSinglesDoubles and qml.BasisRotation we simply changed the argument order. Now, wires is the rightmost positional argument in all three cases.

Benefits: Now, the three Python classes above have an argument order in accordance with pennylane requirements. Additionally, now qml.AllSinglesDoubles also accepts a list as hf_state parameter (not necessarily a numpy array).

Possible Drawbacks: In the case of qml.AllSinglesDoubles and qml.BasisRotation, this is technically a breaking change since we changed the argument order:

import pennylane as qml
import numpy as np

weights = np.ones(3)
singles = [[0, 2], [1, 3]]
doubles = [[0, 1, 2, 3]]
hf_state = [1, 1, 0, 0]
wires=[0, 1, 2, 3]

# works fine
qml.AllSinglesDoubles(weights, hf_state, wires, singles=singles, doubles=doubles)

# works fine
qml.AllSinglesDoubles(weights=weights, wires=wires, hf_state=hf_state, singles=singles, doubles=doubles)

# fails since the second argument is now interpreted as hf_state
# qml.AllSinglesDoubles(weights, wires, hf_state, singles=singles, doubles=doubles)

Related GitHub Issues: #5521

[sc-61430]

@PietropaoloFrisoni PietropaoloFrisoni changed the title Fixing wires as the last positional argument Making wires the last positional argument for several classes May 3, 2024
Copy link

codecov bot commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.68%. Comparing base (756dc62) to head (194f0a2).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5643      +/-   ##
==========================================
- Coverage   99.69%   99.68%   -0.01%     
==========================================
  Files         412      412              
  Lines       38619    38352     -267     
==========================================
- Hits        38500    38233     -267     
  Misses        119      119              

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

Copy link
Contributor

@dwierichs dwierichs left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the quick fix @PietropaoloFrisoni ! 💯

I might have overlooked it, but is there any test where BasisRotation is instantiated with unitary_matrix as positional argument? Might be good to have that in as well, just by changing some arbitrary test. Maybe the test_standard_validity test :)

pennylane/templates/layers/gate_fabric.py Outdated Show resolved Hide resolved
pennylane/templates/layers/gate_fabric.py Show resolved Hide resolved
doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
@PietropaoloFrisoni
Copy link
Contributor Author

PietropaoloFrisoni commented May 3, 2024

Thanks @dwierichs. Actually, I changed the order of the arguments in the tests as well (even where it was not technically necessary) just to be consistent. in test_standard_validity I noticed just now that there was a weights variable instead of unitary_matrix, so I replaced it.

@PietropaoloFrisoni PietropaoloFrisoni marked this pull request as ready for review May 3, 2024 20:40
Copy link
Contributor

@dwierichs dwierichs left a comment

Choose a reason for hiding this comment

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

Thanks @PietropaoloFrisoni I iterated my suggestion. It's good to go from my side either way.

tests/templates/test_subroutines/test_basis_rotation.py Outdated Show resolved Hide resolved
@Alex-Preciado Alex-Preciado requested a review from albi3ro May 6, 2024 15:48
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.

This PR is looking good, and I'm sorry to bring this up right now, but I'd like to propose another option for AllSinglesDoubles.

We coudl instead default hf_state=None, and then if hf_state is None, we simply don't include the BasisState op in the decomposition.

@albi3ro albi3ro requested a review from a team May 7, 2024 19:15
@PietropaoloFrisoni
Copy link
Contributor Author

@albi3ro No problem, I am fine with everything the @PennyLaneAI/algorithms is happy with

@PietropaoloFrisoni
Copy link
Contributor Author

Closing the PR since it has been decided not to fix #5521

PietropaoloFrisoni added a commit that referenced this pull request May 21, 2024
**Context:** Fixing doc typos that I found while working on #5643 (which
has been closed)
@trbromley trbromley deleted the FIxing-wires-last-pos-arg branch August 12, 2024 14:26
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.

3 participants