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

I() for cupy matrices #207

Merged
merged 2 commits into from
Feb 12, 2025
Merged

I() for cupy matrices #207

merged 2 commits into from
Feb 12, 2025

Conversation

BrunoLiegiBastonLiegi
Copy link
Contributor

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi commented Feb 6, 2025

This is an add on for #199, which makes I a cupy array as well. Should also fix the tests of qiboteam/qibo#1548.

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi added the run-on-sim Launch tests on gpu label Feb 6, 2025
Copy link

codecov bot commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (c2b8bda) to head (22b505a).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #207   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines         1121      1121           
=========================================
  Hits          1121      1121           
Flag Coverage Δ
unittests 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi added run-on-sim Launch tests on gpu and removed run-on-sim Launch tests on gpu labels Feb 7, 2025
Copy link

github-actions bot commented Feb 7, 2025

Run on QPU sim completed! :atom:

You can download the coverage report as an artifact, from the workflow summary page:
https://github.com/qiboteam/qibojit/actions/runs/13201379269

Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

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

I was having a look at Qibo to understand why this is needed for cupy.
It appears that the identity is the only matrix in Qibo alongside Unitary that we don't cast using _cast.
https://github.com/qiboteam/qibo/blob/415d98a350af864ca5e02ef52b8ed295138a3b26/src/qibo/backends/npmatrices.py#L66-L67
At this point shouldn't it be better to just cast the identity in Qibo to avoid to redefine it here?
Eventually by casting also Unitary I believe that we can remove also Unitary from CupyMatrices.

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

Yeah, that might be a solution as well. The only counterargument that I would see is that you are introducing a cast that often is not needed, and in some cases it does not matter much because there's not much to do anyway, but for pytorch for instance:
https://github.com/qiboteam/qiboml/blob/96f7c471286192ef0e43295e811878c57d3acafe/src/qiboml/backends/pytorch.py#L26C1-L28C66
is slightly more convoluted.
But yeah, ultimately it should not make a big difference in any case, thus I leave the decision to you guys.

@alecandido
Copy link
Member

I agree we should try to make better use of the standardized array API, and avoiding casting as much as possible.

However, here almost all the matrices are being cast, and only one 2x2 is left plain. I'd say that @andrea-pasquale's proposal is pretty practical...

We're always in time to prevent casting truly leveraging the analogous calls (i.e. what you were doing with CuPy was exactly the same code of the NumPy one), but this is something to properly face in a separate effort.

@andrea-pasquale
Copy link
Contributor

Yeah, that might be a solution as well. The only counterargument that I would see is that you are introducing a cast that often is not needed, and in some cases it does not matter much because there's not much to do anyway, but for pytorch for instance: https://github.com/qiboteam/qiboml/blob/96f7c471286192ef0e43295e811878c57d3acafe/src/qiboml/backends/pytorch.py#L26C1-L28C66 is slightly more convoluted. But yeah, ultimately it should not make a big difference in any case, thus I leave the decision to you guys.

Now that you mention it I remember that when I looked at the Jax backend casting everything was one of the main bottlenecks... From one side as @alecandido said casting everything is more practical but it comes at the cost of worst performances. Therefore either we accept these "worst performances" or if we want better performances we should duplicate the *Matrices for each backend...

For the time being we can merge this PR as it is.

@alecandido
Copy link
Member

alecandido commented Feb 7, 2025

Therefore either we accept these "worst performances" or if we want better performances we should duplicate the *Matrices for each backend...

Actually, what I was mentioning before was a win-win alternative:
https://data-apis.org/array-api/latest/index.html

As you can see:

cp.eye(n, dtype=self.dtype)

is practically the same of

np.eye(n, dtype=self.dtype)

This is often the case for these functions, and not by chance. The project mention above is a joint agreement to standardize this API. I believe NumPy mostly completed his share with NumPy 2.
So, we should reassess which are actually the same, and do the self.np assignment consistently, to make use of this standardization (through which the same source code will be interpreted in different ways, just switching the module).

But, as said, this would take more time, because we also need to check at which point the other projects are in the adoption, and "cast the difference". Let's postpone this.

@MatteoRobbiati MatteoRobbiati merged commit 6f9f63a into main Feb 12, 2025
29 checks passed
@scarrazza scarrazza deleted the cupy_identity_matrix branch February 12, 2025 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-on-sim Launch tests on gpu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants