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

fix expand_matrix for qudit matrices #6398

Merged
merged 15 commits into from
Oct 17, 2024
Merged

fix expand_matrix for qudit matrices #6398

merged 15 commits into from
Oct 17, 2024

Conversation

albi3ro
Copy link
Contributor

@albi3ro albi3ro commented Oct 15, 2024

Context:

We were incorrectly manipulating qutrit matrices.

Description of the Change:

Allow expand_matrix to work on higher qudit matrices.

Benefits:

Possible Drawbacks:

Related GitHub Issues:

[sc-75501] Fixes #6368

@albi3ro albi3ro marked this pull request as ready for review October 15, 2024 21:54
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.40%. Comparing base (e0f614c) to head (0b4a7ee).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6398   +/-   ##
=======================================
  Coverage   99.40%   99.40%           
=======================================
  Files         447      447           
  Lines       42400    42405    +5     
=======================================
+ Hits        42146    42151    +5     
  Misses        254      254           

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

@glassnotes
Copy link
Contributor

@albi3ro and team, thank you so much for making this improvement!! 🎉 🙏 This will be super helpful for our qutrit researchers @jackyruth and @Gabriel-Bottrill 🚀

Copy link
Contributor

@mudit2812 mudit2812 left a comment

Choose a reason for hiding this comment

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

Looks great! Should qml.matrix and Operator.matrix be updated accordingly?

Also, I don't mean to be frustrating but I wanted to get your thoughts on adding a test for qudit_dim > 3 and/or more wires. I guess at least for the former suggestion it might be unnecessary since we don't have any operations for higher qudit dimensions, but the latter might make sense.

@albi3ro
Copy link
Contributor Author

albi3ro commented Oct 16, 2024

Looks great! Should qml.matrix and Operator.matrix be updated accordingly?

Also, I don't mean to be frustrating but I wanted to get your thoughts on adding a test for qudit_dim > 3 and/or more wires. I guess at least for the former suggestion it might be unnecessary since we don't have any operations for higher qudit dimensions, but the latter might make sense.

What do we need to update in qml.matrix or Operator.matrix? Just adding a few more tests?

I'd like to test with more wires, but then we start to run into the challenge with figuring out what the "correct" answer should be 🤔

@mudit2812
Copy link
Contributor

mudit2812 commented Oct 16, 2024

What do we need to update in qml.matrix or Operator.matrix? Just adding a few more tests?

Oh, never mind. The fact that expand_matrix automatically computes the qudit dimension slipped my mind. Though tests aren't a bad idea, but I'd say not necessary either.

I'd like to test with more wires, but then we start to run into the challenge with figuring out what the "correct" answer should be 🤔

Regarding this point, we can either:

  • Use some elbow grease and manually figure out how the matrix should permute. Not ideal, and most likely not worth the labour.
  • Use a mildly less accurate heuristic to assert correctness. For example, we can generate a random matrix and a random state vector and assert that the state vector after applying the matrix is correct regardless of the wire order. This would also allow us to parametrize over arbitrary wire orders and input matrix dimensions to make the test more robust.

@albi3ro albi3ro requested a review from mudit2812 October 17, 2024 13:34
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, just two tiny comments.
Nice! 🚀

pennylane/math/matrix_manipulation.py Outdated Show resolved Hide resolved
pennylane/math/matrix_manipulation.py Show resolved Hide resolved
Copy link
Contributor

@JerryChen97 JerryChen97 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 so much! These days I've been wanting this implemented but didn't get a chance to add; now finally we don't need multiple copies of expansion for any other arbitrary spins in the future😊!!

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
@albi3ro albi3ro merged commit 465d25f into master Oct 17, 2024
37 checks passed
@albi3ro albi3ro deleted the qudit-matrix-fix branch October 17, 2024 17:39
austingmhuang pushed a commit that referenced this pull request Oct 23, 2024
**Context:**

We were incorrectly manipulating qutrit matrices.

**Description of the Change:**

Allow `expand_matrix` to work on higher qudit matrices.

**Benefits:**

**Possible Drawbacks:**

**Related GitHub Issues:**

[sc-75501] Fixes #6368
mudit2812 pushed a commit that referenced this pull request Nov 11, 2024
**Context:**

We were incorrectly manipulating qutrit matrices.

**Description of the Change:**

Allow `expand_matrix` to work on higher qudit matrices.

**Benefits:**

**Possible Drawbacks:**

**Related GitHub Issues:**

[sc-75501] Fixes #6368
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.

[BUG] qml.matrix gives wrong solutions in default.qutrit for more than one wire
6 participants