-
Notifications
You must be signed in to change notification settings - Fork 191
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
Tweak Distributed API #3314
Tweak Distributed API #3314
Conversation
Are there any tests for distributed hydrostatic models? |
yeah in |
@simone-silvestri Oceananigans.jl/src/Oceananigans.jl Lines 221 to 224 in 82503b8
What was "enable overlapping communication" intended to support? Is there something that depends on whether |
There's no tests for Is there another file with tests for |
How important is it to call |
I think users should put the global grid size into the
|
Ok, I found in the docs for MPI.jl that
This is good news. So we can call |
Hopefully we can close #3318 here too, let's discuss. |
validation/distributed_simulations/distributed_geostrophic_adjustment.jl
Outdated
Show resolved
Hide resolved
validation/distributed_simulations/distributed_geostrophic_adjustment.jl
Outdated
Show resolved
Hide resolved
Partition(Rx::Number, Ry::Number=1, Rz::Number=1) = Partition{Rx, Ry, Rz}() | ||
Partition(lengths::Array{Int, 1}) = Partition(reshape(lengths, length(lengths), 1)) | ||
Base.size(::Partition{<:Any, Rx, Ry, Rz}) where {Rx, Ry, Rz} = (Rx, Ry, Rz) | ||
|
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.
Partition(; Rx = 1, Ry = 1, Rz = 1) = Partition(Rx, Ry, Rz) |
here we probably need something like
same for the |
…ustment.jl Co-authored-by: Simone Silvestri <[email protected]>
…ustment.jl Co-authored-by: Simone Silvestri <[email protected]>
I don’t think we should fix the untested features in this PR. We need to merge this ASAP so we can open a new PR with tests. |
@simone-silvestri can you fix the failing test? As soon as tests pass please merge. We need a another PR that implements tests following this one. |
return nothing | ||
end |
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.
return nothing | |
end | |
return nothing | |
end | |
@@ -84,7 +99,7 @@ function LatitudeLongitudeGrid(arch::Distributed, | |||
radius = R_Earth, | |||
halo = (1, 1, 1)) | |||
|
|||
global_size = map(sum, concatenate_local_sizes(size, arch)) | |||
# global_size = map(sum, concatenate_local_sizes(size, arch)) |
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 comment out? delete?
@@ -218,7 +222,7 @@ include("Biogeochemistry.jl") | |||
|
|||
# TODO: move above | |||
include("ImmersedBoundaries/ImmersedBoundaries.jl") | |||
include("DistributedComputations/DistributedComputations.jl") | |||
# include("DistributedComputations/DistributedComputations.jl") |
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 comment out?
@@ -483,8 +485,9 @@ end | |||
@testset "Time stepping ShallowWaterModel" begin | |||
for child_arch in archs | |||
topo = (Periodic, Periodic, Flat) | |||
arch = Distributed(child_arch; ranks=(1, 4, 1), topology = topo, devices = (0, 0, 0, 0)) | |||
grid = RectilinearGrid(arch, topology=topo, size=(8, 2), extent=(1, 2), halo=(3, 3)) | |||
#arch = Distributed(child_arch; ranks=(1, 4, 1), topology = topo, devices = (0, 0, 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.
delete the comment?
|
||
MPI.Barrier(MPI.COMM_WORLD) | ||
# ? |
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.
# ?
?
Co-authored-by: Navid C. Constantinou <[email protected]>
…ceananigans.jl into glw/refactor-distributed
This PR changes up the API for
Distributed
. The changes are:enable_overlapping_computation
which doesn't belong in architectures (which are model agnostic). (We can add that feature to specific models if desired.)MPI.Initialized()
is falseranks
(by default we distribute inx
)Perhaps more to come.
I'm also going to clean up up the examples, since they are the only source of information about running distributed simulations currently.