-
Notifications
You must be signed in to change notification settings - Fork 78
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 measure type error on AMD GPU by passing type on loc function, and also fix broadcasting #150
Conversation
…tions which were failing on the LUMI AMDGPU. Need to check performance.
Codecov ReportAttention: Patch coverage is ❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
|
All tests passing locally on GPU as well. |
These are the benchmarks for master v1.1 before new mutable
|
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.
Looks good. If this is passing the AutoDiff test then you must be doing it right!
@@ -28,18 +28,22 @@ at time `t` using an immersion kernel of size `ϵ`. | |||
See Maertens & Weymouth, doi:[10.1016/j.cma.2014.09.007](https://doi.org/10.1016/j.cma.2014.09.007). | |||
""" | |||
function measure!(a::Flow{N,T},body::AbstractBody;t=zero(T),ϵ=1) where {N,T} | |||
a.V .= 0; a.μ₀ .= 1; a.μ₁ .= 0 | |||
a.V .= zero(T); a.μ₀ .= one(T); a.μ₁ .= zero(T) |
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.
This is the same as fill
, which must maintain the type. So do these actually need to be specified?
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 don't think they need but this is more correct imho.
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.
how can it be more correct if they compile to the same thing and waste characters? ;-)
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.
x)
The ForwardDiff test is only passing for |
I'll run a test on the spinning cylinder example and add a body into the test suite. |
Doesn't work. Failed on |
Huh, I will test on that then and try to reproduce it. |
Let me finish the test case first and push it up |
Finite differences SUCK!
Seems to be working now. I must have had a bug in the bigger version of the code. Current version is a single spinning cylinder. Tested with finite differences (although the FD accuracy is terrible for this case, it converges to the AD result). |
OK great. Note that with this PR the |
When running on LUMI, I came across an error type on
measure
, whereloc
was being used without passing the type. I think in general we should always pass the type when callingloc
so I have fixed this across the codebase.Also, I came across a broadcasting problem on the AMD GPU in LUMI, where operation such as
a[i,:] .= ...
within a kernel are not allowed. To solve this I explicitly wrote the necessary loops in themeasure
function, and the only other place where broadcasting is used within a kernel is inWaterLily.jl/src/Metrics.jl
Line 98 in d23ac35
So probably this would fail too. I will first check any performance regression for
measure
and if all is good we can rewrite the forces routines too.