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 cast method not changing dtype #55

Merged
merged 1 commit into from
Dec 27, 2021
Merged

Fix cast method not changing dtype #55

merged 1 commit into from
Dec 27, 2021

Conversation

mlazzarin
Copy link
Contributor

Closes #47. While working to fix #47, I noticed that PR qiboteam/qibo#528 do not resolve the problem with qibojit.

In fact, the cast method of CuPy doesn't change the dtype if the input array is already a CuPy array. I fixed it, and I also make some additional changes to improve the cast for both CuPy and Numba, because some times the dtype is ignored. Let me know your opinion.

@mlazzarin mlazzarin added the bug Something isn't working label Dec 23, 2021
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.

Looks good to me, thanks. Do you think the default None option for dtype is useful anywhere? If no, we could remove it and use a different default value (eg. "complex128") or not have a default at all and remove the if dtype is None check. This would simplify the code a bit.

@mlazzarin
Copy link
Contributor Author

I'm not sure, self.cast without dtype is called everywhere in backend.py, but I'm not sure if we can simply replace it with DTYPECPX.

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.

I'm not sure, self.cast without dtype is called everywhere in backend.py, but I'm not sure if we can simply replace it with DTYPECPX.

I checked and all cases where self.cast is called without dtype in backend.py are on the state vector, so in principle DTYPECPX would work. The only issue is that I think this is not accessible from this class, only from the global qibojit backend class. Given that dtype=None is also the default for the np.array() call it should be fine to leave it as it is.

Other than that this looks good to me.

@scarrazza
Copy link
Member

Can we merge this?

@mlazzarin
Copy link
Contributor Author

Yes, I believe so.

@scarrazza scarrazza merged commit 00daf26 into main Dec 27, 2021
@stavros11 stavros11 deleted the fixcast branch February 11, 2022 07: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.

Probabilities do not sum to 1
3 participants