-
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
Viscous force and memory allocation #113
Conversation
This fixes #110 by providing a higher-level interface function for the force routines. It also adds a moment computation routine. I will add some proper tests soon. |
src/util.jl
Outdated
@@ -121,8 +121,8 @@ using StaticArrays | |||
Location in space of the cell at CartesianIndex `I` at face `i`. | |||
Using `i=0` returns the cell center s.t. `loc = I`. | |||
""" | |||
@inline loc(i,I::CartesianIndex{N},T=Float64) where N = SVector{N,T}(I.I .- 1.5 .- 0.5 .* δ(i,I).I) | |||
@inline loc(Ii::CartesianIndex,T=Float64) = loc(last(Ii),Base.front(Ii),T) | |||
@inline loc(i,I::CartesianIndex{N}) where N = SVector{N}(I.I .- 3//2 .- 1//2 .* δ(i,I).I) |
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.
Have you checked this works on GPU? It was not working for me on the AD branch.
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.
No, I have not
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.
Ok, it probably won't. We are currently using 1.5f0
... because otherwise it doesn't work in GPUs.
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 will revert to the type conversion of the SVector for now. We can sort this out later.
There are currently no tests for |
I can add a test for that! |
Co-authored-by: Bernat Font <[email protected]>
…ations. Note that the reduction (sum) is always performed over Float64 type. Updated tests and examples with the new functions.
All tests passing locally on CPU and GPU. @marinlauber the modifications I did to your previous changes (up to bb99599) make all the metrics work for GPU now, since that implementation was only working for CPU. The allocations are very good as well (there are just a few more, but the routines are also a bit faster):
The only remaining thing to discuss is the examples added. Only the TwoD_cylinderVIV.jl had to be changed to comply with this PR, and the other examples do not actually even use any force/moment metric. I suggest we remove those changes from this PR and create a new one just for examples. Then this PR should be ready to merge. |
@weymouth can you please give it a look before we merge to master? |
…examples. These examples do not use force metrics so they should be submitted in a different PR.
Are we ok with discontinuing the |
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, except for that one default, which I can fix.
src/AutoBody.jl
Outdated
@@ -112,6 +112,11 @@ function measure(a::Bodies,x,t) | |||
sdf, map, _ = sdf_map_d(a.bodies,a.ops,x,t) | |||
measure(sdf,map,x,t) | |||
end | |||
# measures the distance, normal, and velocity accuratly for sdf≤1 | |||
function measure_fast(body::AutoBody,x,t) | |||
abs(body.sdf(x,t))>1 && return zero(eltype(x)),zero(x),zero(x) |
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.
Why doesn't this return d, zero(x), zero(x)
?
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's an error, should return d.
Yeap, we will just keep the high-level functions since it was difficult to do multiple dispatch of |
Which one? |
The
The price to pay for having these functions AD-compatible is fine though (plus with KA in CPU or GPU the difference is very small!) :) |
This implements a simple viscous force routine. Following the structure of the routine for the pressure force, which needs to include the time of the simulation to make sure we are measuring at the correct time, see #110;
Also, it is allocating like crazy, same as for the pressure force routine.
This is a work in progress.