-
Notifications
You must be signed in to change notification settings - Fork 40
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 C++ decomposition with scipy_openblas32
#995
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #995 +/- ##
==========================================
- Coverage 94.44% 92.20% -2.24%
==========================================
Files 229 178 -51
Lines 38841 27390 -11451
==========================================
- Hits 36682 25254 -11428
+ Misses 2159 2136 -23 ☔ View full report in Codecov by Sentry. |
awesome work @multiphaseCFD ! Just added some comments/questions, happy to look over it again and approve once answered! |
also have you updated the changelog? |
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.
Thank you @multiphaseCFD
Only a couple of comments 👍
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.
LGTM! 🏆 Happy to approve after addressing the last round of concerns and suggestions.
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 @multiphaseCFD! 🎉
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.
LGTM, thanks @multiphaseCFD for this addition!
Before submitting
Please complete the following checklist when submitting a PR:
All new features must include a unit test.
If you've fixed a bug or added code that should be tested, add a test to the
tests
directory!All new functions and code must be clearly commented and documented.
If you do make documentation changes, make sure that the docs build and
render correctly by running
make docs
.Ensure that the test suite passes, by running
make test
.Add a new entry to the
.github/CHANGELOG.md
file, summarizing thechange, and including a link back to the PR.
Ensure that code is properly formatted by running
make format
.When all the above are checked, delete everything above the dashed
line and fill in the pull request template.
Context:
[sc-78187] & [sc-53754]
The
blas/lapack
support is a long standing issue forlightning
. The main challenge is the wheels building for thewindows
platform, which does not support afortran
compiler natively. Recently, thescipy
team release the standalonescipy-openblas32
wheel forlinux
,windows
andmacos
platform. This could solve the win support issue.There are a few technical discussions on how to add the
scipy-openblas32
support tolightning
:relative path
to thelibscipy_openblas
is added to therunpath
of the shared lib object of installations from wheel. In this case,scipy-openblas32
is required to be installed in the samesite-packages
dir. However, for the direct C++ usage as well aspip editable install
, the absolute path tolibscipy_openblas
is still needed to ensure thelibscipy_openblas
lib can be found and loaded.scipy-openblas32
is a dependency oflightning
.conda-forge
.This PR also brings a few other changes:
BLASLib
loading is handled by a singleton classBLASLibLoaderManager
, instead of thecompute_diagonalizing_gates
API.openblas
support is tested againstmacos
,linux
andwindows
for the C++ tests on CIs and integration withcatalyst
locally.cmake option
:PY_INSTALL
is added to identify if the pip installation is editable or not. ThisPY_INSTALL
isON
ifnot self.editable_mode
.Description of the Change:
Benefits:
Possible Drawbacks:
Related GitHub Issues: