Add restart support for masked spaces #2212
Conversation
charleskawczynski
force-pushed
the
ck/read_data
branch
from
71e2b37 to
39b620e
Compare
charleskawczynski
changed the title
charleskawczynski
force-pushed
the
ck/read_data
branch
4 times, most recently
from
4a97767 to
d8c730e
Compare
| !(topology isa Topologies.Topology2D && !(writer.context isa ClimaComms.SingletonCommsContext)) | ||
| ``` | ||
| """ | ||
| function _write_mpi!( |
There was a problem hiding this comment.
I haven't gone through the PR in details yet, but this caught my eye. When InputOutput writes other objects (including Fields and their associated data) it can directly handle the distributed case. Why is there a need to distinguish the MPI and non-MPI case here?
Also,
!(topology isa Topologies.Topology2D && !(writer.context isa ClimaComms.SingletonCommsContext))
The double negation makes this hard to understand. It seems that this says that the function should be used when the topology is not 2D and it is singleton, the opposite of what the function name suggests. Is this a typo?
There was a problem hiding this comment.
Okay, I looked at the existing code and think I have answered my own question about the need for two functions: they are not really handled directly and there are still if-else statements
There was a problem hiding this comment.
Yeah, I wish there was a better way, but you really need to know topology information to handle datalayout IO correctly. I split the method into two pieces in case either of them could be reused. For now, I'm only using these methods for the mask but we could eventually reuse them in other places to reduce some of the duplicate logic.
I think happy to remove the double negative from the docs, I tend to avoid them, but I think I just did that to somewhat match the source code.
|
Could you add some notes in the PR description about the main changes being made here to support restarts with masked spaces? It would probably be good to add something like that to the NEWS too |
charleskawczynski
force-pushed
the
ck/read_data
branch
from
d8c730e to
7cecf0e
Compare
| "grid_mask", | ||
| write!(writer, grid.mask, Spaces.topology(grid)), | ||
| ) | ||
| # write!(writer, grid.mask, Spaces.topology(grid)) |
There was a problem hiding this comment.
| # write!(writer, grid.mask, Spaces.topology(grid)) |
| ) | ||
| group = create_group(writer.file, "grid_mask/$name") | ||
| write!(writer, group, mask.is_active, "is_active", topology) | ||
| name |
There was a problem hiding this comment.
| name | |
| return name |
| topology::Topologies.AbstractTopology, | ||
| ) | ||
|
|
||
| Write an object of type 'AbstractData' and name 'name' to the HDF5 file. |
There was a problem hiding this comment.
| Write an object of type 'AbstractData' and name 'name' to the HDF5 file. | |
| Write an object of type `AbstractData` and name `name` to the HDF5 file. |
| This method should be used when | ||
|
|
||
| ```julia | ||
| !(topology isa Topologies.Topology2D && !(writer.context isa ClimaComms.SingletonCommsContext)) |
There was a problem hiding this comment.
Is this restriction on Topology 2D needed because we don't support MPI for Topology1D?
There was a problem hiding this comment.
Also, I think this conditional is incorrect for this case and should just be
topology isa Topologies.Topology2D && !(writer.context isa ClimaComms.SingletonCommsContext)
There was a problem hiding this comment.
Is this restriction on Topology 2D needed because we don't support MPI for Topology1D?
I think that is true, but I used this condition mostly to preserve the existing behavior
| This method should be used when | ||
|
|
||
| ```julia | ||
| !(topology isa Topologies.Topology2D && !(writer.context isa ClimaComms.SingletonCommsContext)) |
There was a problem hiding this comment.
I find this hard to read (because of the double/nested negation). I'd suggest
| !(topology isa Topologies.Topology2D && !(writer.context isa ClimaComms.SingletonCommsContext)) | |
| is_distributed = !(writer.context isa ClimaComms.SingletonCommsContext) | |
| !is_distributed || !(topology isa Topologies.Topology2D) |
| function _write!(group, values::DataLayouts.AbstractData, name::AbstractString;) | ||
| h_dim = DataLayouts.h_dim(DataLayouts.singleton(values)) | ||
| array = parent(values) | ||
| # dataset = write_plain_array!(writer, array, "data/$name") |
There was a problem hiding this comment.
| # dataset = write_plain_array!(writer, array, "data/$name") |
| context = ClimaComms.SingletonCommsContext(device) | ||
| grid_topology = Topologies.Topology2D(context, mesh) | ||
| context = ClimaComms.context(device) |
There was a problem hiding this comment.
I think this test only works for SingletonCommsContext.
If you use MPI, mktempfile will create two different files on in the /tmp/ of different nodes.
There was a problem hiding this comment.
Right, I need to think about this.
| Spaces.set_mask!(space) do coords | ||
| rand() > 0.5 | ||
| end | ||
| is_active₀ = Spaces.get_mask(space).is_active |
There was a problem hiding this comment.
This (the is_active₀) doesn't seem to be used anywhere
There was a problem hiding this comment.
Ah, right, I think this is sort of a rebase artifact
Closes #2216