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

Parallelization of CliffordOperations through numba and cupy #161

Merged
merged 61 commits into from
Feb 28, 2024

Conversation

BrunoLiegiBastonLiegi
Copy link
Contributor

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi commented Dec 13, 2023

This PR implements the CliffordOperations object introduced in qiboteam/qibo#1076 to be used specifically with the NumbaBackend and CupyBackend. Namely, each operation is rewritten as a loop over the row indices that is parallelized using, prange and compiled with @njit(parallel=True, cache=True) for numba, and @jit.rawkernel() with cupy.

Companion of qiboteam/qibo#1150

@renatomello renatomello self-requested a review December 13, 2023 14:42
@renatomello renatomello added the enhancement New feature or request label Dec 13, 2023
@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi changed the title Parallelization of CliffordOperations through numba Parallelization of CliffordOperations through numba and cupy Dec 19, 2023
@renatomello renatomello changed the title Parallelization of CliffordOperations through numba and cupy Parallelization of CliffordOperations through numba and cupy Jan 10, 2024
@alecandido
Copy link
Member

Should we silence Codecov in Qibojit?

@alecandido
Copy link
Member

Should we silence Codecov in Qibojit?

I had a second thought, since tests are present in Qibojit (though in an unusual location wrt the other repos), and there is a related workflow.
https://github.com/qiboteam/qibojit/tree/main/src/qibojit/tests
https://github.com/qiboteam/qibojit/actions/workflows/rules.yml

But I get that you wrote the related tests already in Qibo.
Eventually, we will disentangle the dependencies, but this is part of the qibo-core effort (and I'd like to merge these PRs before, if possible immediately).

Before silencing Codecov, or skip tests for this PR, it could be valuable to collect the opinions of the original Qibojit authors. @scarrazza @stavros11 @andrea-pasquale

@andrea-pasquale
Copy link
Contributor

andrea-pasquale commented Feb 22, 2024

I know that in the past there were plans to test directly on gpu through a selfhosted workflow. I believe that it should be less complicated compare to qibolab tests. If we can do so, we should be able to recover coverage, right?
If it is too complicated or if we feel like that it is a bit of an overkill I am also happy to drop coverage and eventually skip tests.

Comment on lines +711 to +726
def _clifford_pre_execution_reshape(state):
return state.ravel()


def _clifford_post_execution_reshape(state, nqubits):
dim = _get_dim(nqubits)
return state.reshape(dim, dim)


def identity_density_matrix(nqubits, normalize: bool = True):
n = 1 << nqubits
state = cp.eye(n, dtype="complex128")
cp.cuda.stream.get_current_stream().synchronize()
if normalize:
state /= 2**nqubits
return state.reshape((n, n))
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I'd say that using cp.ravel and cp.reshape is more robust just in case (I know, I know...)

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

BrunoLiegiBastonLiegi commented Feb 26, 2024

@renatomello @alecandido @scarrazza is this ready to be merged? I would merge this one first and then the corresponding one in qibo.

Copy link
Contributor

@renatomello renatomello left a comment

Choose a reason for hiding this comment

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

Is the 0% coverage because it needs the PR in qibo too?

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

Is the 0% coverage because it needs the PR in qibo too?

It's because the changes here are tested in qibo, as the cupy and numba clifford operations are defined here but used inside of the CliffordBackend.

@renatomello
Copy link
Contributor

renatomello commented Feb 26, 2024

Is the 0% coverage because it needs the PR in qibo too?

It's because the changes here are tested in qibo, as the cupy and numba clifford operations are defined here but used inside of the CliffordBackend.

So then maybe they shouldn't count to coverage here (maybe # pragma: no cover everywhere or some other way to exclude entire files?)

@alecandido
Copy link
Member

alecandido commented Feb 26, 2024

To be completely honest: things implemented here should be tested here (as for every other project).

Having features spread over many repos is an issue on its own. But we are starting facing that issue with qiboteam/qibo#1117 and https://github.com/qiboteam/qibo-core/, so I will not insist in this PR.

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

To be completely honest: things implemented here should be tested here (as for every other project).

Having features spread over many repos is an issue on its own. But we are starting facing that issue with qiboteam/qibo#1117 and https://github.com/qiboteam/qibo-core/, so I will not insist in this PR.

I agree, but this would involve copy pasting the tests that are currently implemented in qibo here. If you think it's better I can do that, it was just to avoid code duplication.

@alecandido
Copy link
Member

I agree, but this would involve copy pasting the tests that are currently implemented in qibo here. If you think it's better I can do that, it was just to avoid code duplication.

I believe the best would be to move all this code to Qibo, possibly to the quantum_info module, where Clifford is defined in the first place.
But let's merge this PR, and consider this an optimization.

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi merged commit 524aea0 into main Feb 28, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants