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

Removing the wire_map argument from qml.is_commuting #5660

Merged
merged 7 commits into from
May 8, 2024

Conversation

PietropaoloFrisoni
Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni commented May 6, 2024

Context: Part of deprecations and removals for pennylane-0.37

Description of the Change: Removed the wire_map argument from qml.is_commuting.

[sc-58988]

@PietropaoloFrisoni PietropaoloFrisoni marked this pull request as ready for review May 6, 2024 20:47
@trbromley
Copy link
Contributor

@glassnotes @josh146, looking at the commit history you were both involved in adding qml.is_commuting. Do you know why we added a wire_map? Our thinking has been that a mapping of wires does not alter whether two operations commute, but I want to check if we're missing something.

@josh146
Copy link
Member

josh146 commented May 7, 2024

@trbromley I'm sorry, I don't recall the reasoning here :( Do you have a link to the original PR?

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 makes me happy :)

@albi3ro
Copy link
Contributor

albi3ro commented May 7, 2024

@trbromley I'm sorry, I don't recall the reasoning here :( Do you have a link to the original PR?

This was added in #3033 because grouping.utils.is_commuting used it, and grouping.utils.is_commuting used it because it converted the observables to binary form. We no longer used said binary form because it isn't as efficient as using the pauli rep. And even if we ever did use a binary form, we could just infer the wire order from the observables...

@glassnotes
Copy link
Contributor

@glassnotes @josh146, looking at the commit history you were both involved in adding qml.is_commuting. Do you know why we added a wire_map? Our thinking has been that a mapping of wires does not alter whether two operations commute, but I want to check if we're missing something.

Yeah I think it was added to support taking the symplectic inner product to evaluate commutativity, and the functions that convert to binary symplectic rep required a wire order.

Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.68%. Comparing base (ab8d739) to head (7ed4392).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5660      +/-   ##
==========================================
- Coverage   99.69%   99.68%   -0.02%     
==========================================
  Files         412      412              
  Lines       38663    38367     -296     
==========================================
- Hits        38545    38245     -300     
- Misses        118      122       +4     

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

@trbromley
Copy link
Contributor

Thanks everyone! Ok, looks like a wire map is no longer needed here and we'll proceed with removing it.

@PietropaoloFrisoni
Copy link
Contributor Author

PietropaoloFrisoni commented May 8, 2024

Thanks everyone. I'll merge this PR as soon as I have a second approval

Copy link
Contributor

@astralcai astralcai left a comment

Choose a reason for hiding this comment

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

Don't forget to update the deprecations page as well

doc/releases/changelog-dev.md Show resolved Hide resolved
@PietropaoloFrisoni
Copy link
Contributor Author

@astralcai I left a comment about that above. I'll repost it here;

As mentioned on Shortcut, it has been decided not to go through a deprecation cycle because the presence of wire_map is not functional and can be viewed more as a bug than a feature.

Therefore, the doc/development/deprecations.rst file has not been modified in this PR.

@PietropaoloFrisoni PietropaoloFrisoni enabled auto-merge (squash) May 8, 2024 19:33
@PietropaoloFrisoni PietropaoloFrisoni merged commit e9a554a into master May 8, 2024
38 checks passed
@PietropaoloFrisoni PietropaoloFrisoni deleted the Removing_wire_map_from_is_commuting branch May 8, 2024 20:52
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.

6 participants