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

Backend-agnostic matrix definitions #1217

Merged
merged 12 commits into from
Feb 16, 2024
Merged

Backend-agnostic matrix definitions #1217

merged 12 commits into from
Feb 16, 2024

Conversation

Simone-Bordoni
Copy link
Contributor

@Simone-Bordoni Simone-Bordoni commented Feb 13, 2024

Store a backend agnostic matrix definition tht can be casted with a specific _cast function in the proper backend.
Necessary for PR #1202

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

@Simone-Bordoni Simone-Bordoni added the enhancement New feature or request label Feb 13, 2024
@Simone-Bordoni Simone-Bordoni self-assigned this Feb 13, 2024
@renatomello renatomello changed the title backend agnostic matrix definitions Backend-agnostic matrix definitions Feb 14, 2024
@Simone-Bordoni
Copy link
Contributor Author

@renatomello
I am getting an error for the CSXDG gate.
previously is was:

self.np.conj(self.CSX)

Now i changed to:

a = (1 - 1j) / 2
b = self.np.conj(a)
      return self._cast(
          [
              [1, 0, 0, 0],
              [0, 1, 0, 0],
              [0, 0, a, b],
              [0, 0, b, a],
          ],
          dtype=self.dtype,
      )

It should be the same thing as CSX is defined the same way but with a=(1+1j)/2

@renatomello
Copy link
Contributor

@renatomello I am getting an error for the CSXDG gate. previously is was:

self.np.conj(self.CSX)

Now i changed to:

a = (1 - 1j) / 2
b = self.np.conj(a)
      return self._cast(
          [
              [1, 0, 0, 0],
              [0, 1, 0, 0],
              [0, 0, a, b],
              [0, 0, b, a],
          ],
          dtype=self.dtype,
      )

It should be the same thing as CSX is defined the same way but with a=(1+1j)/2

Which error though

@Simone-Bordoni
Copy link
Contributor Author

@renatomello I am getting an error for the CSXDG gate. previously is was:

self.np.conj(self.CSX)

Now i changed to:

a = (1 - 1j) / 2
b = self.np.conj(a)
      return self._cast(
          [
              [1, 0, 0, 0],
              [0, 1, 0, 0],
              [0, 0, a, b],
              [0, 0, b, a],
          ],
          dtype=self.dtype,
      )

It should be the same thing as CSX is defined the same way but with a=(1+1j)/2

Which error though

Assertion error in test_csxdg and test_dagger (the tests that failed also on github)

@renatomello
Copy link
Contributor

Which error though

Assertion error in test_csxdg and test_dagger (the tests that failed also on github)

It's only failing for numba. So, most likely a PR in qibojit is needed here adding

@cached_property
def CSXDG(self):
    return self.SXDG

I have not tested locally but I'd imagine this would fix it.

@stavros11
Copy link
Member

stavros11 commented Feb 14, 2024

It's only failing for numba. So, most likely a PR in qibojit is needed here adding

@cached_property
def CSXDG(self):
    return self.SXDG

I have not tested locally but I'd imagine this would fix it.

@renatomello do you have any idea why this is the case? I understand this PR is changing the calculations of some matrices a bit, but the returned values should remain the same. I don't see how this would break qibojit. Unless this was an existing issue that came up now for some reason.

@renatomello
Copy link
Contributor

renatomello commented Feb 14, 2024

@renatomello do you have any idea why this is the case? I understand this PR is changing the calculations of some matrices a bit, but the returned values should remain the same. I don't see how this would break qibojit. Unless this was an existing issue that came up now for some reason.

I didn't dig much deeper, but I checked and numba is generating the same matrices as the other backends (via gates.Gate.matrix(backend)), but somehow when it applies the gate and its dagger to a state it does not recover the original state. At least numpy and tensorflow are passing this test for this gate. I didn't check cupy / cuquantum.
And yes, I agree this is strange and should've came up in other situations.

@Simone-Bordoni
Copy link
Contributor Author

Which error though

Assertion error in test_csxdg and test_dagger (the tests that failed also on github)

It's only failing for numba. So, most likely a PR in qibojit is needed here adding

@cached_property
def CSXDG(self):
    return self.SXDG

I have not tested locally but I'd imagine this would fix it.

I have opened the PR in qibojit

@Simone-Bordoni
Copy link
Contributor Author

@renatomello @stavros11 by adding the CSXDG gate in qibojit the tests fail only on linux...

@renatomello
Copy link
Contributor

renatomello commented Feb 14, 2024

@renatomello @stavros11 by adding the CSXDG gate in qibojit the tests fail only on linux...

it's because only linux is testing tensorflow, and the issue is with other gates

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5fa9c82) 100.00% compared to head (98cf3d3) 100.00%.

❗ Current head 98cf3d3 differs from pull request most recent head bfaf142. Consider uploading reports for the commit bfaf142 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1217   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           70        70           
  Lines        10379     10381    +2     
=========================================
+ Hits         10379     10381    +2     
Flag Coverage Δ
unittests 100.00% <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.

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.

This looks good to advance #1202, but it needs to be merged with qiboteam/qibojit#168.

It'd be also interesting to test at least locally if this is really enough for pytorch to work.

@Simone-Bordoni
Copy link
Contributor Author

Now that the qibojit branch "add CSXDG" has been merged this PR is ready to be merged (after one missing review)

@renatomello renatomello added this pull request to the merge queue Feb 16, 2024
Merged via the queue into master with commit d9d9413 Feb 16, 2024
19 checks passed
@renatomello renatomello deleted the np branch February 16, 2024 08:52
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.

3 participants