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

Update the unification work required for the external package development #482

Merged
merged 10 commits into from
Sep 7, 2023

Conversation

maliasadi
Copy link
Member

@maliasadi maliasadi commented Aug 23, 2023

This PR fixes the following issues.

  1. The Lightning Monorepo doesn’t consider dependent external package development:
    • The core libraries are not interfaced and cannot be accessed
    • The simulator libraries are not interfaced and cannot be accessed.
  2. Catalyst extends StateVectorLQubit by implementing a managed-like state-vector class with dynamic qubit management. To add this support,
    • The num_qubits_ needs to be a "protected" class variable so that it can be updated dynamically in StateVectorLQubitDynamic.
    • The implementation of simulators’ algorithms, observables, measurements should be updated to support any concrete implementation of StateVectorLQubit.

[sc-43458]

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +6.04% 🎉

Comparison is base (869bbb8) 93.04% compared to head (60aad8b) 99.09%.
Report is 2 commits behind head on master.

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     
Files Changed Coverage Δ
...tning/core/src/simulators/base/StateVectorBase.hpp 100.00% <ø> (ø)
...tning_qubit/gates/tests/Test_OpToMemberFuncPtr.cpp 18.46% <ø> (ø)
pennylane_lightning/core/_version.py 100.00% <100.00%> (ø)
.../simulators/lightning_kokkos/StateVectorKokkos.hpp 99.76% <100.00%> (+5.99%) ⬆️
...s/gates/tests/Test_StateVectorKokkos_Generator.cpp 100.00% <100.00%> (ø)
...os/gates/tests/Test_StateVectorKokkos_NonParam.cpp 100.00% <100.00%> (ø)
...okkos/gates/tests/Test_StateVectorKokkos_Param.cpp 100.00% <100.00%> (ø)
...s/lightning_kokkos/measurements/ExpValFunctors.hpp 100.00% <100.00%> (+43.06%) ⬆️
...ghtning_kokkos/measurements/MeasurementsKokkos.hpp 98.26% <100.00%> (+3.79%) ⬆️
...asurements/tests/Test_StateVectorKokkos_Expval.cpp 100.00% <100.00%> (ø)
... and 5 more

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

Copy link
Member

@mlxd mlxd left a 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.

@maliasadi maliasadi changed the title 🚧 Update the unification work required for the external package development Update the unification work required for the external package development Aug 29, 2023
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.

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.

Makefile Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@mlxd
Copy link
Member

mlxd commented Sep 7, 2023

Hey @maliasadi feel free to retag us for review once this is updated.

Copy link
Member

@mlxd mlxd left a 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

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.

Happy to have this in, if this will help PennyLane catalyst!

@maliasadi maliasadi merged commit 44c3c0e into master Sep 7, 2023
60 checks passed
@maliasadi maliasadi deleted the catalyst-deps-2 branch September 7, 2023 17:53
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.

4 participants