MITgcm icon indicating copy to clipboard operation
MITgcm copied to clipboard

cleaning tamc.h : collect previous comments

Open jm-c opened this issue 3 years ago • 4 comments

In PR #579 discussion, removing some un-used parameters within "tamc.h" was mentionned but postponed until "nthreads_chkpt" could also be removed (after removing it from tape initialisations). Raise this issue here to give this task more visibility (+ also as a reminder).

The relevant part in the discussion were: https://github.com/MITgcm/MITgcm/pull/579#issuecomment-1001553299 and https://github.com/MITgcm/MITgcm/pull/579#issuecomment-1003640663

jm-c avatar Feb 22 '22 16:02 jm-c

@jm-c could you please explain, for reference, why nthreads_chkpt should never be different from 1? Is It because the number of threads (i.e. nTx*nTy) always needs to be < nSx*nSy, and since nSx and nSy are already considered in the size of the tapes?

mjlosch avatar Sep 08 '22 06:09 mjlosch

I suggest a (draft) PR where we remove nthreads_chkpt from the_main_loop.F (the only place where it is used) and agree on the form of the reference tamc.h and then adjust all 19 verification-tamc.h (and the verification_other ones). I can start this PR if desired.

mjlosch avatar Sep 08 '22 13:09 mjlosch

Regarding nthreads_chkpt , you are right: each time it appears in the code (in: the_main_loop.F and in pkg/smooth/smooth_diff2/3d.F) it's multiplied by nSx*nSy. But whether we run single thread or mutlple threads, the size of multi-tile arrays don't change, always nSx*nSy tiles per proc. So there is no need to reserve more space than the array size. And in the case where we would need to store a common block array such as the ones in pkg/aim_v23/ (e.g., in AIM_GRID.h, com_forcing.h or com_physvar.h) with the last dimension function of the maximum number of threads "MAX_NO_THREADS" instead of the number of tiles "nSx*nSy", we would not use the product of both but instead either one or the other (MAX_NO_THREADS or nSx*nSy).

jm-c avatar Sep 13 '22 16:09 jm-c

@mjlosch I agree with your plan for a draft PR.

jm-c avatar Sep 13 '22 16:09 jm-c

An other simplification we could do when computing "key" to store a single tile array: The key setting involves the tile indices bi,bj (as it should), but in a complicated way, for instance, see https://github.com/MITgcm/MITgcm/blob/1575dc0796db30b8393db6d59dc55153dfa104b0/model/src/convective_adjustment.F#L104 grouping the tiles by thread. This ordering is not necessary (if we were running multi-threads, they would not run sequentially anyway) and we could just use simply the tile indices bi,bj: ikey = bi + (bj - 1)*nSx + (ikey_dynamics - 1)*nSx*nSy

jm-c avatar Nov 08 '22 19:11 jm-c

Thanks for this suggestion! The key computation was always hard for my to understand, but I never dared to touch this in fear of breaking something that was well tested. There are a few places where I introduced new key computations, and I already did them in the way similar to your suggestions, e.g. here: https://github.com/MITgcm/MITgcm/blob/1575dc0796db30b8393db6d59dc55153dfa104b0/pkg/thsice/thsice_advection.F#L276

So I am all for it!

mjlosch avatar Nov 09 '22 09:11 mjlosch