nextsimhub / nextsimdg

neXtSIM_DG : next generation sea-ice model with DG
https://nextsim-dg.readthedocs.io/en/latest/?badge=latest
Apache License 2.0
10 stars 13 forks source link

Issue513 rect grid test improvements #514

Closed draenog closed 6 months ago

draenog commented 6 months ago

Improvements to RectGrid parallelisation

Fixes #513

The pull requests implements the following workload in RectGrid_test:

  1. Runs test file in on 1 MPI rank with known data.
  2. Reads the same file in parallel and compare that the data are the same.
  3. Writes the same file in parallel so it can be compared with the file written serially.

The resulting field is still different as currently RectGridIO::getModelState does not read sst and sss components so they are missing form the resulting file.

Additionally the pull requests:

  1. Tests local dimensions in parallel case
  2. Introduce tests for ModelArray::Type::Z dimensions
  3. Fixes reading of 3D data in paralle mode
  4. Fix setting dimensions in parallel case
  5. Tests local dimensions in parallel case
  6. Introduce tests for ModelArray::Type::Z dimensions
draenog commented 6 months ago

@timspainNERSC Could you maybe comment on this:

The resulting field is still different as currently RectGridIO::getModelState does not read sst and sss components so they are missing form the resulting file.

Is it on purpose or should RectGridIO::getModelState read ssst and sssdata?

timspainNERSC commented 6 months ago

@timspainNERSC Could you maybe comment on this:

The resulting field is still different as currently RectGridIO::getModelState does not read sst and sss components so they are missing form the resulting file.

Is it on purpose or should RectGridIO::getModelState read ssst and sssdata?

When I wrote RectGridIO, we hadn't looked at the slab ocean, which is what uses sss and sst. These were added to ParaGridIO when I added the slab ocean, but I never went back and added them to RectGridIO. Since RectGrid is not intended to be used in the production model (it doesn't support the dynamics at all), I'm not planning on going back and adding support. Unless told otherwise, of course.

draenog commented 6 months ago

@timspainNERSC Could you maybe comment on this:

The resulting field is still different as currently RectGridIO::getModelState does not read sst and sss components so they are missing form the resulting file.

Is it on purpose or should RectGridIO::getModelState read ssst and sssdata?

When I wrote RectGridIO, we hadn't looked at the slab ocean, which is what uses sss and sst. These were added to ParaGridIO when I added the slab ocean, but I never went back and added them to RectGridIO. Since RectGrid is not intended to be used in the production model (it doesn't support the dynamics at all), I'm not planning on going back and adding support. Unless told otherwise, of course.

In this case I wonder if it would be worth removing these two field from ModelState in RectGrid_test.cpp. That way if the state is written, read and written again it should produce exactly the same file which is currently not the case.

draenog commented 6 months ago

This looks good to me. But could you just briefly clarify the relationship between nDims possibly being larger then 2, and the partition file and bounding box etc only having x and y dimensions?

This comes from the design of parallelization that the domain only in x and y directions. I am not sure if the test is a good place to document it.

MarionBWeinzierl commented 6 months ago

@timspainNERSC Could you maybe comment on this:

The resulting field is still different as currently RectGridIO::getModelState does not read sst and sss components so they are missing form the resulting file.

Is it on purpose or should RectGridIO::getModelState read ssst and sssdata?

When I wrote RectGridIO, we hadn't looked at the slab ocean, which is what uses sss and sst. These were added to ParaGridIO when I added the slab ocean, but I never went back and added them to RectGridIO. Since RectGrid is not intended to be used in the production model (it doesn't support the dynamics at all), I'm not planning on going back and adding support. Unless told otherwise, of course.

In this case I wonder if it would be worth removing these two field from ModelState in RectGrid_test.cpp. That way if the state is written, read and written again it should produce exactly the same file which is currently not the case.

@timspainNERSC , would that be ok?

timspainNERSC commented 6 months ago

@timspainNERSC Could you maybe comment on this:

The resulting field is still different as currently RectGridIO::getModelState does not read sst and sss components so they are missing form the resulting file.

Is it on purpose or should RectGridIO::getModelState read ssst and sssdata?

When I wrote RectGridIO, we hadn't looked at the slab ocean, which is what uses sss and sst. These were added to ParaGridIO when I added the slab ocean, but I never went back and added them to RectGridIO. Since RectGrid is not intended to be used in the production model (it doesn't support the dynamics at all), I'm not planning on going back and adding support. Unless told otherwise, of course.

In this case I wonder if it would be worth removing these two field from ModelState in RectGrid_test.cpp. That way if the state is written, read and written again it should produce exactly the same file which is currently not the case.

@timspainNERSC , would that be ok?

Yes, that sounds fine.

draenog commented 6 months ago

I've just pushed the updated branch. The changes:

  1. Rebases on develop commit 1423515cd293a978de5d050dfafadc91756e50e0 to fix CI tests on Mac OS.
  2. Remove sss and sst fields from ModelState as they are not read by ParaGridIO
  3. Remove hardcoded offsets in test spotted by @MarionBWeinzierl