-
Notifications
You must be signed in to change notification settings - Fork 42
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
Update the unification work required for the external package development #482
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #482 +/- ##
==========================================
+ Coverage 93.04% 99.09% +6.04%
==========================================
Files 142 142
Lines 16278 16693 +415
==========================================
+ Hits 15146 16542 +1396
+ Misses 1132 151 -981
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the changes are added, I'll approve.
Also, the RTD builder is failing now. There is a PR needed to fix this open at #491 which will be necessary before merging.
pennylane_lightning/core/src/simulators/base/StateVectorBase.hpp
Outdated
Show resolved
Hide resolved
pennylane_lightning/core/src/simulators/lightning_qubit/utils/LinearAlgebra.hpp
Outdated
Show resolved
Hide resolved
pennylane_lightning/core/src/simulators/lightning_qubit/utils/LinearAlgebra.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @maliasadi, I'm happy with having num_qubits
changed to protected.
In terms of algorithms, measurements and observables, I would rather have an explicit reference/check with respect to the StateVectorLQubitCatalist
than a catch-all that assumes that everything that is not StateVectorLQubitRaw
will be StateVectorLQubitManaged
-like.
This brings us to another topic. In terms of all we discussed, and continuous support for StateVectorLQubitCatalist
, I think it would be a good call to have it added as a new StateVectorLQubit
variant to Lightning. In this way, our CI/CD will make sure no change breaks it. For example, having num_qubits
as private works very well for Lightning's needs so far, if we have a backend relying on it the team will always remember (or be remembered of) this constraint.
pennylane_lightning/core/src/simulators/lightning_qubit/algorithms/AdjointJacobianLQubit.hpp
Outdated
Show resolved
Hide resolved
Hey @maliasadi feel free to retag us for review once this is updated. |
5e07db9
to
a7fcf1d
Compare
…nto catalyst-deps-2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @maliasadi
Fine with me to see this go through if it meets your needs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to have this in, if this will help PennyLane catalyst!
This PR fixes the following issues.
StateVectorLQubit
by implementing a managed-like state-vector class with dynamic qubit management. To add this support,num_qubits_
needs to be a "protected" class variable so that it can be updated dynamically inStateVectorLQubitDynamic
.StateVectorLQubit
.[sc-43458]