-
Notifications
You must be signed in to change notification settings - Fork 9
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
Release v3.0 and fix CI for newer Julia versions #37
Conversation
Codecov Report
@@ Coverage Diff @@
## master #37 +/- ##
==========================================
+ Coverage 88.25% 89.70% +1.45%
==========================================
Files 3 3
Lines 298 340 +42
==========================================
+ Hits 263 305 +42
Misses 35 35
Continue to review full report at Codecov.
|
This passed pointers to copies of the input arrays to finufft_setpts. The copies could then be garbage collected at random, leaving the pointers invalid
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.
If this change can pass all the tests if we switch back to hcat and original rnd. Then it can explain that the bug is that the copied arrays may be garbage collected at random.
I think we should keep the change to random number generation, as it removes any dependency on what is the underlying default random number generator. This makes the tests more reproducible, and more likely to pass across different environments. |
@@ -1,7 +1,7 @@ | |||
name = "FINUFFT" | |||
uuid = "d8beea63-0952-562e-9c6a-8e8ef7364055" | |||
author = "Ludvig af Klinteberg <[email protected]>" |
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.
do you still get email at KTH? :)
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.
Haha, yes so far they haven't shut down the address :D
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.
All looks good to me. The only changes I see are to fix issue #34, change the RNG, update CI to 1.6 + 1.7, and remove some type-conversions. Is that all that was needed? Thanks, Alex
@@ -1,7 +1,7 @@ | |||
name = "FINUFFT" | |||
uuid = "d8beea63-0952-562e-9c6a-8e8ef7364055" | |||
author = "Ludvig af Klinteberg <[email protected]>" | |||
version = "2.1.0" | |||
version = "3.0.0" |
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.
Are you bumping up the major number since it changes the interface (a bit)? It still is an interface to FINUFFT 2.0.3. I wonder if that is confusing? I guess no more than right now :)
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.
Yep, just released v3.0. We'll just have to live with the slight weirdness of package and interface running at two different versions
@@ -170,10 +169,9 @@ function finufft_setpts(plan::finufft_plan{T}, | |||
Ref{Cdouble}, | |||
Ref{Cdouble}, | |||
Ref{Cdouble}), | |||
plan.plan_ptr,M,T_real(xj),T_real(yj),T_real(zj),N,T_real(s),T_real(t),T_real(u) | |||
plan.plan_ptr, M, xj, yj, zj, N, s, t, u |
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.
makes sense to simplify, but I wonder why Libin put in the conversions in the first place?
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.
makes sense to simplify, but I wonder why Libin put in the conversions in the first place?
I think I was trying to do a array sanity check as in python interface, https://github.com/flatironinstitute/finufft/blob/4fe380544202f0ad7b89f52c4e08a70f090ed22b/python/finufft/_interfaces.py#L175
While here in Julia we enforce the type check in function signature, so the type conversion is not necessary.
I ran the test using pkg add FINUFFT#julia-1.7 |
Great, thanks for explaining - yes, a good feature of Julia :)
…On Wed, Jan 5, 2022 at 5:11 PM Libin Lu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/guru.jl
<#37 (comment)>:
> @@ -170,10 +169,9 @@ function finufft_setpts(plan::finufft_plan{T},
Ref{Cdouble},
Ref{Cdouble},
Ref{Cdouble}),
- plan.plan_ptr,M,T_real(xj),T_real(yj),T_real(zj),N,T_real(s),T_real(t),T_real(u)
+ plan.plan_ptr, M, xj, yj, zj, N, s, t, u
makes sense to simplify, but I wonder why Libin put in the conversions in
the first place?
I think I was trying to do a array sanity check as in python interface,
https://github.com/flatironinstitute/finufft/blob/4fe380544202f0ad7b89f52c4e08a70f090ed22b/python/finufft/_interfaces.py#L175
While here in Julia we enforce the type check in function signature, so
the type conversion is not necessary.
—
Reply to this email directly, view it on GitHub
<#37 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACNZRSWKYUXB3I3UXH73CBTUUS6Z5ANCNFSM5LG3X3CA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
--
*---------------------------------------------------------------------~^`^~._.~'
|\ Alex H. Barnett Center for Computational Mathematics, Flatiron
Institute
| \ http://users.flatironinstitute.org/~ahb
646-876-5942
|
No description provided.