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

Reimplement qubit unitary #6986

Merged
merged 23 commits into from
Feb 26, 2025

Conversation

JerryChen97
Copy link
Contributor

Context:
Our current sparse repr for QubitUntiary is quite confusing: its matrix() return a sparse matrix by default, if this obj was instantiated by a CSR, which is inconsistent from what we expected for other operators, that matrix() should return a dense and sparse_matrix() gives out the sparse, well separated from each other.

Therefore, a slight re-work is needed to make things clearer. Basically, we will overload the has_matrix and has_sparse from parents and have a more friendly and straightforward usage.

Description of the Change:

Benefits:

Possible Drawbacks:

Related GitHub Issues:

@JerryChen97 JerryChen97 changed the base branch from master to e2e-sparse-default-qubit February 20, 2025 19:10
@JerryChen97 JerryChen97 self-assigned this Feb 20, 2025
@JerryChen97 JerryChen97 marked this pull request as ready for review February 20, 2025 22:07
@AmintorDusko
Copy link
Contributor

@JerryChen97, are these CI tests supposed to fail?
Is there anything you can do to fix that?

@JerryChen97
Copy link
Contributor Author

@JerryChen97, are these CI tests supposed to fail? Is there anything you can do to fix that?

Sorry, forgot to fix them on e2e branch. I'll fix soon

@JerryChen97
Copy link
Contributor Author

External fail is exactly the same as this morning. Just ignore it until we merge this PR to e2e branch and update there with master

JerryChen97 added a commit that referenced this pull request Feb 21, 2025
**Context:**
In our on-going sparse matrix epic, we used to postponed the full
implementation of controlled ops family due to others of higher
priorities. However, in a related PR
https://github.com/PennyLaneAI/pennylane/actions/runs/13464002433/job/37625640042?pr=6986
it seems inevitable to have the `wire_order` branch of
`Controlled.sparse_matrix` method implemented.

For the sake of traceability, and to prevent some the mentioned PR from
being furthermore spaghettized with high entanglement, we implement the
`wire_order` branch here with unit tests, and watch and fix whatever new
impact introduced as an independent PR instead of hotfix overthere.

**Description of the Change:**
Direct call `expand_matrix` just as what `matrix()` did above
`sparse_matrix`, right before format-convert.
Convert the error-raise test into a consistency-check with
`matrix(wire_order=...)`

**Benefits:**
Fully fledged sparse `Controlled`

Unblock #6986

**Possible Drawbacks:**
Small chance to still have incompatibility in the future, which is also
why we would like to have this separated such that easy to revert.

**Related GitHub Issues:**
[sc-84949]

---------

Co-authored-by: Andrija Paurevic <[email protected]>
@JerryChen97 JerryChen97 requested a review from a team as a code owner February 24, 2025 16:02
@JerryChen97 JerryChen97 removed the request for review from a team February 24, 2025 16:02
@JerryChen97
Copy link
Contributor Author

😅Messed up with the git branch control

@JerryChen97 JerryChen97 force-pushed the reimplement-QubitUnitary branch from cfaa2aa to 7a24f16 Compare February 24, 2025 16:38
Copy link

codecov bot commented Feb 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (e2e-sparse-default-qubit@9c0728c). Learn more about missing BASE report.

Additional details and impacted files
@@                     Coverage Diff                     @@
##             e2e-sparse-default-qubit    #6986   +/-   ##
===========================================================
  Coverage                            ?   99.59%           
===========================================================
  Files                               ?      483           
  Lines                               ?    46156           
  Branches                            ?        0           
===========================================================
  Hits                                ?    45971           
  Misses                              ?      185           
  Partials                            ?        0           

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

@JerryChen97
Copy link
Contributor Author

@AmintorDusko @albi3ro Now we have cleared the errors. Major changes since last check: 1. Controlled has its sparse_matrix fully implemented (merged from master) 2. the condition for apply_operation to dispatch into csr branch changed to having both has_sparse_matrix==True and has_matrix==False, s.t. we don't wrongly dispatch into csr for those operators that used to have both, e.g. RX, RY etc.

@AmintorDusko
Copy link
Contributor

Hey @JerryChen97, we still have some work to do regarding this error.

Copy link
Contributor

@AmintorDusko AmintorDusko left a comment

Choose a reason for hiding this comment

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

Nice job! I don't see any blocker but I have a few questions.

@AmintorDusko
Copy link
Contributor

I wonder if you know what is happening with PennyLane wheels here.

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.

Thanks for that change 👍

@JerryChen97
Copy link
Contributor Author

I wonder if you know what is happening with PennyLane wheels here.

PL CI affected by setuptools updates. Fix coming soon.

@JerryChen97
Copy link
Contributor Author

CI back and passed

Copy link
Contributor

@AmintorDusko AmintorDusko left a comment

Choose a reason for hiding this comment

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

Nice job!

@JerryChen97 JerryChen97 merged commit ad1946d into e2e-sparse-default-qubit Feb 26, 2025
41 checks passed
@JerryChen97 JerryChen97 deleted the reimplement-QubitUnitary branch February 26, 2025 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