CICE-Consortium / CICE

Development repository for the CICE sea-ice model
Other
57 stars 131 forks source link

Update the automated max_blocks calculation #954

Closed apcraig closed 4 months ago

apcraig commented 4 months ago

PR checklist

apcraig commented 4 months ago

I still testing, refining the test suite, and updating documentation. But this should represent the code changes I'm proposing. Things are running well. The max_blocks=-1 setting now computes the maximum required blocks on each task and sets the internal max_blocks variable to that value. That means that it uses exactly the amount of memory required and the max_blocks can vary per task. Users can still manually set max_blocks in namelist as before.

apcraig commented 4 months ago

Testing results look good. https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#7402dc7f04f98d840890f29f8f02a59f956a8fc2.

This is ready for review and merge.

apcraig commented 4 months ago

Could someone do a review on this PR? Would love to get this merged. Then I can start comprehensively testing in preparation for a release. Thanks!

dabail10 commented 4 months ago

There is a lot here, so I might have missed something. I'm not going to get a chance to test this out until later (after the workshop). I will approve, but just know I might find stuff later once I have tested.

eclare108213 commented 4 months ago

@anton-seaice do you have time to look at this? It's probably after hours there...

apcraig commented 4 months ago

I have update the PR based on feedback from @anton-seaice and am running a set of tests just to make sure nothing is broken. Will report results when the testing is done. Thanks @anton-seaice for the comments.

apcraig commented 4 months ago

I reran a portion of the test suite with the latest code changes and I think everything is OK. I'll merge once github actions passes and @anton-seaice is happy with the current implementation. Please let me know if anything else needs to be fixed. Thanks!

anton-seaice commented 4 months ago

There just a couple if lines in ice_domain_size that are not totally consistent now:

    max_blocks  , & ! max number of blocks per processor

Could be updated

   !*** The model will inform the user of the correct
   !*** values for the parameter below.  A value higher than
   !*** necessary will not cause the code to fail, but will
   !*** allocate more memory than is necessary.  A value that
   !*** is too low will cause the code to exit.
   !*** A good initial guess is found using
   !*** max_blocks = (nx_global/block_size_x)*(ny_global/block_size_y)/
   !***               num_procs

Can probably be removed because its covered in the docs ?

apcraig commented 4 months ago

There just a couple if lines in ice_domain_size that are not totally consistent now:

    max_blocks  , & ! max number of blocks per processor

Could be updated

   !*** The model will inform the user of the correct
   !*** values for the parameter below.  A value higher than
   !*** necessary will not cause the code to fail, but will
   !*** allocate more memory than is necessary.  A value that
   !*** is too low will cause the code to exit.
   !*** A good initial guess is found using
   !*** max_blocks = (nx_global/block_size_x)*(ny_global/block_size_y)/
   !***               num_procs

Can probably be removed because its covered in the docs ?

good catch, fixed these.

anton-seaice commented 4 months ago

good catch, fixed these.

I think you still need to push the commit