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

Add Hermitian Support for Finite-Shot Measurement Statistics #451

Open
wants to merge 135 commits into
base: main
Choose a base branch
from

Conversation

multiphaseCFD
Copy link
Member

@multiphaseCFD multiphaseCFD commented Jan 10, 2024

Context:

Description of the Change:

Benefits:

Possible Drawbacks:

Shortcut Stories:
[sc-53754]

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.95%. Comparing base (bd09fe1) to head (71af0cf).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #451      +/-   ##
==========================================
- Coverage   99.95%   97.95%   -2.01%     
==========================================
  Files          20       71      +51     
  Lines        4439    10505    +6066     
  Branches        0      886     +886     
==========================================
+ Hits         4437    10290    +5853     
- Misses          2      170     +168     
- Partials        0       45      +45     

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

@maliasadi maliasadi added runtime Pull requests that update the runtime frontend Pull requests that update the frontend labels Jan 12, 2024
@multiphaseCFD multiphaseCFD marked this pull request as ready for review January 15, 2024 19:11
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.

Thank you @multiphaseCFD 🥳

I think we also need to make sure that our wheels are built correctly. I would imagine that we need to add the lapack library to our binary distribution, unless you link it into lightning statically? Or does lightning look for it at runtime?

We also need to ensure the change works on MacOS (installing the liblapack-dev equivalent etc), running the wheel build action for mac from this branch would be a good test for it.

@@ -38,7 +38,7 @@ jobs:

- name: Install deps
run: |
sudo apt-get install -y cmake ninja-build ccache libomp-dev libasan6
sudo apt-get install -y cmake ninja-build ccache libomp-dev liblapack-dev libasan6
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're adding the package here it probably needs to be added to all our other actions as well (where appropriate).
Our install docs also need to reflect this new dependency:

Building from source

runtime/Makefile Outdated
@@ -14,7 +14,8 @@ ENABLE_LIGHTNING?=ON
ENABLE_LIGHTNING_KOKKOS?=OFF
ENABLE_OPENQASM?=OFF
ENABLE_ASAN?=OFF
LIGHTNING_GIT_TAG_VALUE?="v0.34.0"
ENABLE_LAPACK?=ON
LIGHTNING_GIT_TAG_VALUE?="master"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we target the minimum commit on the master branch that introduced support? Having a fixed hash will allow us to go back through Catalyst's git history and still build it successfully.

runtime/Makefile Outdated
@@ -14,7 +14,8 @@ ENABLE_LIGHTNING?=ON
ENABLE_LIGHTNING_KOKKOS?=OFF
ENABLE_OPENQASM?=OFF
ENABLE_ASAN?=OFF
LIGHTNING_GIT_TAG_VALUE?="v0.34.0"
ENABLE_LAPACK?=ON
Copy link
Contributor

Choose a reason for hiding this comment

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

This option is added but unused?

@multiphaseCFD
Copy link
Member Author

Thanks @dime10 ! Per packaging/wheels building, we should add RPATH (path\to\liblapack or scipy) to cmake. RPATH embeds information in the binary about where to find its dependent shared libraries at runtime. We've already use it to fix DSO errors for L-GPU when linking against custatevec.so.

@dime10
Copy link
Contributor

dime10 commented Jan 22, 2024

Thanks @dime10 ! Per packaging/wheels building, we should add RPATH (path\to\liblapack or scipy) to cmake. RPATH embeds information in the binary about where to find its dependent shared libraries at runtime. We've already use it to fix DSO errors for L-GPU when linking against custatevec.so.

The one problem I see with this is that for deployed packages (like a Catalyst wheel), the correct RPATH (on the user's system) is not known at build time.

@Alex-Preciado Alex-Preciado changed the title add Hermitian support to shot-noise measurement Add Hermitian Support to shot-noise Measurement Feb 3, 2024
@Alex-Preciado Alex-Preciado changed the title Add Hermitian Support to shot-noise Measurement Add Hermitian Support for Finite-Shot Measurement Statistics Feb 3, 2024
@multiphaseCFD multiphaseCFD added author:build-wheels Run the wheel building workflows on this Pull Request and removed author:build-wheels Run the wheel building workflows on this Pull Request labels Mar 21, 2024
@multiphaseCFD multiphaseCFD added the author:build-wheels Run the wheel building workflows on this Pull Request label Mar 21, 2024
@multiphaseCFD multiphaseCFD added the author:build-wheels Run the wheel building workflows on this Pull Request label Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author:build-wheels Run the wheel building workflows on this Pull Request frontend Pull requests that update the frontend runtime Pull requests that update the runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants