-
Notifications
You must be signed in to change notification settings - Fork 90
Uses complex dtypes in cufinufft py tests #705
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
Uses complex dtypes in cufinufft py tests #705
Conversation
bbd0bce
to
dd50854
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think it's good, just need to make the new merged type3 tests run successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems jenkins failed on the newly merged test_cufinufft3_simple
test https://jenkins.flatironinstitute.org/blue/organizations/jenkins/finufft/detail/PR-705/4/pipeline
I guess we need to merge master to this branch do the change for test_cufinufft3_simple
too.
And real_dtype
seems not used anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we need to merge master to this branch do the change for
test_cufinufft3_simple
too.
Ok I rebaed on top of master and made the changes. Let's see how the tests do.
And
real_dtype
seems not used anywhere.
I don't quite understand. It's used to generate the correct dtypes for the frequencies, no? See here:
finufft/python/cufinufft/tests/utils.py
Line 52 in c9e6a27
k = gen_nu_pts(M, dim=dim).astype(real_dtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks, all the jenkins tests pass. I think it's good to merge.
I just happen to see the two variables real_dtype
defined in file test_simple.py
are not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. I've removed them now. Thanks!
Will wait for tests to finish and then merge.
ccba36c
to
c9e6a27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks, all the jenkins tests pass. I think it's good to merge.
I just happen to see the two variables real_dtype
defined in file test_simple.py
are not used.
Fixes #413.