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

Remove SVDynamic #999

Merged
merged 9 commits into from
Nov 20, 2024
Merged

Remove SVDynamic #999

merged 9 commits into from
Nov 20, 2024

Conversation

rauletorresc
Copy link
Contributor

@rauletorresc rauletorresc commented Nov 19, 2024

Context: When the LQ plugin was migrated from Catalyst to Lightning, the StateVectorLQDynamic class was also migrated. Given the fact that we have StateVectorLQManaged, we can get rid of StateVectorLQDynamic.

Description of the Change: Replace the uses of StateVectorLQDynamic with StateVectorLQManaged. I basically reused the code from the LK simulator and removed the calls to the StateVectorLQDynamic related methods. Of course, I had to use std::complex instead of kokkos::complex.

Benefits: Using the same underlying class for LG, LK and LQ.

NOTE: I have deleted the only two tests that were failing:

  1. Qubit allocatation and deallocation (https://github.com/PennyLaneAI/pennylane-lightning/pull/999/files#diff-4782c8d1aa88463716419ff1ef4f9f551d6241e43f5bc107d27ff0ed4299a3ebL78): the second call to State(...) showed a size of 2 for the given state and a size of 4 for the device state, which made the assertion fail.
  2. QuantumDevice object test (https://github.com/PennyLaneAI/pennylane-lightning/pull/999/files#diff-4782c8d1aa88463716419ff1ef4f9f551d6241e43f5bc107d27ff0ed4299a3ebL193) The test was killed after 1 minute of running, probably because an infinite loop/recursion?

It seems to me these two tests do not comply with the way StateVectorLQManaged is structured but let the discussion resolve it.

[sc-77689]

@rauletorresc rauletorresc self-assigned this Nov 19, 2024
@rauletorresc rauletorresc marked this pull request as ready for review November 19, 2024 00:36
Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.74%. Comparing base (ad205ee) to head (b784213).
Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (ad205ee) and HEAD (b784213). Click for more details.

HEAD has 26 uploads less than BASE
Flag BASE (ad205ee) HEAD (b784213)
33 7
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #999      +/-   ##
==========================================
- Coverage   97.74%   91.74%   -6.01%     
==========================================
  Files         231      176      -55     
  Lines       36767    24507   -12260     
==========================================
- Hits        35938    22483   -13455     
- Misses        829     2024    +1195     

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


🚨 Try these New Features:

@rauletorresc rauletorresc requested review from a team, dime10 and mlxd November 19, 2024 03:52
Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

Nice work @rauletorresc! 🥳

.github/CHANGELOG.md Outdated Show resolved Hide resolved
@maliasadi maliasadi added the ci:build_wheels Activate wheel building. label Nov 19, 2024
Copy link
Contributor

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

Just a question for Ali:

@rauletorresc rauletorresc merged commit 4c8d840 into master Nov 20, 2024
95 of 96 checks passed
@rauletorresc rauletorresc deleted the raultorres/remove_SV_dynamic branch November 20, 2024 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:build_wheels Activate wheel building.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants