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 multiqubit GPU ops not called by JitCustomBackend #41

Merged
merged 2 commits into from
Nov 13, 2021

Conversation

mlazzarin
Copy link
Contributor

It looks like we forgot to update the call to the multiqubit GPU kernels in JitCustomBackend after we moved the latter from qibo to qibojit.

@mlazzarin mlazzarin added the bug Something isn't working label Nov 13, 2021
@codecov
Copy link

codecov bot commented Nov 13, 2021

Codecov Report

Merging #41 (25999f1) into main (4c41a1d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #41   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          983       978    -5     
=========================================
- Hits           983       978    -5     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

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

Impacted Files Coverage Δ
src/qibojit/custom_operators/__init__.py 100.00% <100.00%> (ø)
src/qibojit/custom_operators/gates.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c41a1d...25999f1. Read the comment docs.

@mlazzarin
Copy link
Contributor Author

I also made some changes in gates.py to improve readability. Let me know if you agree.

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

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

Thanks for finding this! Both the fix and the readability changes look good to me.

I just hope that the previous fallback to CPU did not have any effect to the tests/benchmarks we were doing (thinking that we run on GPU but actually use CPU), although at least for the benchmarks I believe we would notice the difference from the results, particularly for high qibut numbers. Now it works in my machine and I can confirm that the multi-qubit GPU kernel is called during testing.

@scarrazza
Copy link
Member

@mlazzarin thanks for these updates and fixes, looks good to me.

@scarrazza scarrazza merged commit 27934dd into main Nov 13, 2021
@mlazzarin
Copy link
Contributor Author

I just hope that the previous fallback to CPU did not have any effect to the tests/benchmarks we were doing (thinking that we run on GPU but actually use CPU), although at least for the benchmarks I believe we would notice the difference from the results, particularly for high qibut numbers.

@stavros11 I think the benchmarks of PR #23 and PR #37 are fine, because the fallback was removed in commit 58e1dfe and re-introduced only in commit 3183df2, and the benchmarks were performed in the middle of the two commits. We may need to re-run the tests with the different configurations, though, but I usually do it every time a create a new environment, so it should be fine.

@scarrazza scarrazza deleted the fixmultiqubitgpu branch February 11, 2022 17:43
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.

3 participants