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

Fix root cause of bug reported in PR #1412 #1417

Merged
merged 2 commits into from
Aug 14, 2024
Merged

Conversation

renatomello
Copy link
Contributor

@renatomello renatomello commented Aug 8, 2024

The root cause of the issue raised in #1412 was a cast to numpy inside NumpyBackend.samples_to_binary. That is what was turning the subsequent cast into a requirement for torch. Without the former, the latter could be removed.

Checklist:

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

@renatomello renatomello changed the title Fix root cause of bug reported in #1404 Fix root cause of bug reported in #1412 Aug 8, 2024
@renatomello renatomello changed the title Fix root cause of bug reported in #1412 Fix root cause of bug reported in PR #1412 Aug 8, 2024
@renatomello renatomello linked an issue Aug 8, 2024 that may be closed by this pull request
@renatomello renatomello self-assigned this Aug 8, 2024
qrange = np.arange(nqubits - 1, -1, -1, dtype=np.int32)
samples = self.to_numpy(samples)
return np.mod(np.right_shift(samples[:, None], qrange), 2)
Copy link
Member

Choose a reason for hiding this comment

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

Aren't all the other operations in here still NumPy? Or is Torch automatically converting NumPy calls?

I expected self.np. usage, but instead there is np. everywhere...

Copy link
Contributor Author

@renatomello renatomello Aug 8, 2024

Choose a reason for hiding this comment

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

I tried self.np and it creates other issues because torch.arange and torch.right_shift work slightly differently

Copy link
Member

Choose a reason for hiding this comment

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

But is np.right_shift() able to work on Torch tensors? (i.e. taking Torch tensors as input and returning them as output)

I see it could be vaguely possible, but it's still good to know, because I wouldn't have been confident to say it's actually able.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In [6]: import numpy as np
   ...: import torch
   ...: 
   ...: x = torch.Tensor(torch.arange(10))
   ...: qrange = np.arange(10 - 1, -1, -1, dtype=np.int32)
   ...: type(np.right_shift(x[:, None], qrange))
Out[6]: torch.Tensor

Copy link
Member

Choose a reason for hiding this comment

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

Nice, thanks :)

Copy link
Member

@alecandido alecandido Aug 8, 2024

Choose a reason for hiding this comment

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

It seems that by now this starts being reliable

https://numpy.org/doc/stable/user/basics.interoperability.html#operating-on-foreign-objects-without-converting
https://numpy.org/neps/nep-0018-array-function-protocol.html#nep18

(ok, most likely since a while, but it is for sure framework-dependent, I should take the time of assessing which ones are fully supporting it)

@renatomello renatomello added the bug Something isn't working label Aug 8, 2024
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (3d87ba7) to head (ad3eb17).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1417      +/-   ##
==========================================
- Coverage   99.94%   99.94%   -0.01%     
==========================================
  Files          78       78              
  Lines       11225    11224       -1     
==========================================
- Hits        11219    11218       -1     
  Misses          6        6              
Flag Coverage Δ
unittests 99.94% <100.00%> (-0.01%) ⬇️

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.

@renatomello renatomello requested a review from alecandido August 9, 2024 05:07
@scarrazza scarrazza added this pull request to the merge queue Aug 14, 2024
Merged via the queue into master with commit 37e1010 Aug 14, 2024
27 checks passed
@renatomello renatomello deleted the fix-complex-samples branch August 14, 2024 09:22
@alecandido alecandido mentioned this pull request Aug 30, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MeasurementOutcomes samples return type
3 participants