Change icetmask to logical consistent with iceumask, icenmask, iceemask #773
Conversation
- Add icetmask as logical array to ice_grid.F90, was integer array - Update use of icetmask in code for consistency with new type - Add ice_HaloUpdate2DL1 to support halo updates for logical fields in both mpi and serial ice_boundary.F90 - Modify some capital T,U,N,E in ice_dyn_shared.F90 to t,u,n,e for better consistency in code
apcraig
requested review from
eclare108213,
dabail10,
TillRasmussen and
phil-blain
|
Without running this pull request it seems fine. It could be considered whether icetmask and iceumask should be allocated and defined within each of the dynamical drivers and by doing this keep the dynamical driver more independent of the rest of the code. There are other variables where this could be applied as well. |
| @@ -2384,6 +2385,69 @@ subroutine ice_HaloUpdate2DI4(array, halo, & | |||
|
|
|||
| end subroutine ice_HaloUpdate2DI4 | |||
|
|
|||
| !*********************************************************************** | |||
|
|
|||
| subroutine ice_HaloUpdate2DL1(array, halo, & | |||
There was a problem hiding this comment.
I think I'd name it ice_HaloUpdate2DL since there's no concepts of "precision" for logicals, so we do not add any information by having this trailing 1.
There was a problem hiding this comment.
It's a good point and I thought about that when I implemented it. I guess I like that all the subroutine names are as similar as possible, even if it doesn't make sense always. But happy to change this if there is consensus. This interface name is not directly used, so it's not really critical either way.
There was a problem hiding this comment.
I'm ambivalent. The 1 is fine...
There was a problem hiding this comment.
Lets leave it as is.
Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
| @@ -2384,6 +2385,69 @@ subroutine ice_HaloUpdate2DI4(array, halo, & | |||
|
|
|||
| end subroutine ice_HaloUpdate2DI4 | |||
|
|
|||
| !*********************************************************************** | |||
|
|
|||
| subroutine ice_HaloUpdate2DL1(array, halo, & | |||
There was a problem hiding this comment.
I'm ambivalent. The 1 is fine...
| indxNj ! compressed index in j-direction | ||
| indxei , & ! compressed index in i-direction | ||
| indxej , & ! compressed index in j-direction | ||
| indxni , & ! compressed index in i-direction |
There was a problem hiding this comment.
I think the capitalization helps with interpreting the variables when reading the code, because it indicates grid locations. E.g. we use lower-case n for thickness categories, and I've always used lower-case t for time and lower-case u (and v) for velocity. But I won't quibble if others prefer lower case for these.
There was a problem hiding this comment.
I go back and forth. I changed a bunch of variables names a few months back in ice_dyn_shared, but they are not consistent with the rest of the code now. I guess I thought we might change them in other places, but that hasn't happened. I think we should move to capital T, U, N, E, but I think consistency in the code is generally more important, so I changed some of these back. There is a lot we could still do to clean this up if we wanted.
There was a problem hiding this comment.
If we ultimately want to move to capitals for these kinds of things, then I'd advocate for keeping the capitals here and, whenever there's time/energy (not necessarily this PR), fix the others to be consistent.
There was a problem hiding this comment.
OK, I'll undo these changes before we merge, no problem.
|
I moved ice[T,U,N,E]mask into ice_dyn_shared.F90, thanks @TillRasmussen for prodding me. I think getting those variables out of ice_grid is good. I also updated some dynamics variables to capitalize T,U,N,E consistently across dynamics. Everything is still passing and bit-for-bit. Will resolve new conflicts and retest. |
|
Everything is bit-for-bit with the current main, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#34a9f239347b5a6f1c08ebf9b3d2dd54509a724a, and ready for merge after further review. |
PR checklist
Short (1 sentence) summary of your PR:
Change icetmask to a logical array
Developer(s):
apcraig
Suggest PR reviewers from list in the column to the right.
Please copy the PR test results link or provide a summary of testing completed below.
All tests are bit-for-bit on cheyenne with 3 compilers, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#80e0e532888ab339c8bdac47a37256aae804aba2
How much do the PR code changes differ from the unmodified code?
Does this PR create or have dependencies on Icepack or any other models?
Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.
Does this PR add any new test cases?
Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
Please provide any additional information or relevant details below:
Add icetmask as logical array to ice_grid.F90, was integer array
Update use of icetmask in code for consistency with new type
Add ice_HaloUpdate2DL1 to support halo updates for logical fields in both mpi and serial ice_boundary.F90
Modify some capital T,U,N,E in ice_dyn_shared.F90 to t,u,n,e for better consistency in code