Remove unnecessary transfer of variable values in call to solve_noahowp_grid

[GreyREvenson]

PURPOSE

This PR is intended to correct for issue #76, which identified that many variable values were being unnecessarily transferred from the noahowpgrid_type subtypes (i.e., the noahowpgrid_type member derived data types such as domaingrid_type) to the noahowp_type subtypes (i.e., the noahowp_type member derived data types such as domain_type) and back again.

ADDITIONS

InitTransfer type-bound private subroutines were added to the noahowp_type subtypes and are called within the noahowp_type subtype's existing public Init subroutines.

The added InitTransfer subroutines require an instance of the corresponding noahowpgrid_type subtype as an argument. Thus, the InitTransfer procedures are called using the following syntax:

• levels_type%InitTransfer(levelsgrid_type)
• domain_type%InitTransfer(domaingrid_type)
• options_type%InitTransfer(optionsgrid_type)
• parameters_type%InitTransfer(parametersgrid_type)
• water_type%InitTransfer(watergrid_type)
• forcing_type%InitTransfer(forcinggrid_type)
• energy_type%InitTransfer(energygrid_type)

The above subroutines transfer variable values from the argument instance of the noahowpgrid_type subtype to the calling instance of the noahowp_type subtype. These subroutines are responsible for transferring variables that are spatially-constant -- i.e., variables do not vary through space and therefore do not have x and y indices as defined in its corresponding noahowpgrid_type subtypes).We are transferring/setting spatially constant variables during the scope of the Init subroutines because there is no need to set/re-set these values for every grid-cell within the solve_noahowp_grid spatial-loop.

Note that the added InitTransfer type-bound subroutines previously existed within the code base but were moved/recycled into the InitTransfer type-bound subroutines that are now associated with the noahowpgrid_type subtypes.

CHANGES

With every call to solve_noahowp_grid, a new local instance of noahowp_type is defined and its subtype Init procedures must be called. These Init procedures were revised to have two required arguments in (1) an instance of namelist_type, and (2) an instance of the corresponding noahowpgrid_type subtype. Thus, noahowp_type subtype Init subroutines are now called with the following syntax:

• levels_type%Init(namelist_type, levelsgrid_type)
• domain_type%Init(namelist_type, domaingrid_type)
• options_type%Init(namelist_type, optionsgrid_type)
• parameters_type%Init(namelist_type, parametersgrid_type)
• water_type%Init(namelist_type, watergrid_type)
• forcing_type%Init(namelist_type, forcinggrid_type)
• energy_type%Init(namelist_type, energygrid_type)

The noahowp_type subtype public Init subroutines call the subtype's private InitAllocate, InitDefault, and InitTransfer subroutines, which are responsible for allocating the noahowp_type subtype's allocatable arrays (if any), initializing values to huge(), and transferring spatially constant variables from the argument noahowpgrid_type subtype.

Within solve_noahowp_grid, all variable values are necessarily transferred into the noahowp_type subtypes prior to calling solve_noahowp; however, it is not necessary to transfer all variables out of the noahowp_type subtypes and back to the noahowpgrid_type subtypes because not all variables are changed within the scope of solve_noahowp. I removed unnecessary transferring-out of variables values within the ParametersVarOutTransfer and DomainVarOutTransfer subroutines. The LevelsVarOutTransfer and OptionsVarOutTransfer subroutines are empty (i.e., they do nothing).

TESTING

The model was compiled and executed in standalone mode.

Inputs and forcing for grid cell (1,1) match those given in namelist.input in the main branch of this repo. Thus, outputs for grid cell (1,1) should match those provided a clean build/run of the code base in the main branch. test/analysis/compare_outputnc.py was executed to confirm. All tests passed for grid cell (1,1) -- indicating that all outputs for this cell matched.

NOTES

[GreyREvenson]

Leaving some comments to help communicate my reasoning.

[GreyREvenson]

Removing obsolete comment

[GreyREvenson]

Removing transfer of variables that are now transferred in newly added domain_type%InitTransfer subroutine (see src/DomainType.f90)

[GreyREvenson]

Transfer-in for these variables has been moved to forcing_type%InitTransfer (see ForcingType.f90)

[GreyREvenson]

Adding ix and iy as arguments for sake of consistency across the transfer subroutines, even though they are not used here.

[GreyREvenson]

Giving ix and iy to the transfer functions for options_type for sake of consistency. They are not used.

[GreyREvenson]

Making namelist_type are required argument for levels_type%Init to be consistent across all Init functions bound to the noahowp_type's member derived data types.

[GreyREvenson]

Make namelist_type a required argument for forcing_type%Init to be consistent across all Init subroutines bound to the noahowp_type's member derived data types.

[GreyREvenson]

Make namelist_type a required argument for options_type%Init to be consistent across all Init subroutines bound to the noahowp_type's member derived data types.

[GreyREvenson]

idt removed because it wasn't being used.

[GreyREvenson]

Removed because these are no longer used.

[nmizukami]

I like the consistent pattern for all the types as done here, and I don't find any line by line comments.

A few overall comments (just my opinion)

I might be missing some details on why it is structured now, but I feel that it would be possible to have only one procedure - Init to initialize the object with either default value or from grid information.

For now, most of Init routine calls InitDefault. maybe if you supply an argument optionsgrid (use optional argument?) in Init routine, transfer the data from optionsgrid, otherwise, initialize with default values.

So Idea is to have only one public procedure that does either depending on whether the argument is provided or not.

Would you expect other ways to initialize the type in the future?

[GreyREvenson]

Thanks for having a look, @nmizukami.

I don't expect other ways of initialization so your suggestion makes good sense to me. I was aiming to replicate the mechanics of the noahowp_type initialization procedures in the main branch, which has the Init and InitTransfer routines as public. Without being able to speak to Keith et al. about why they did things this way, I'd just guess that the approach offers better 'separation of concerns'. However, I also see the logic of your suggestion and I'd guess that it's generally preferable to reduce the number of public routines (?).

It would be good to get @SnowHydrology's thoughts on this, when he's back in the office. I started working on this issue while he was gone so I need to circle back to make sure we're on the same page.

[andywood]

I think the separation of Init and InitTransfer InitDefault etc in Noah-OM was not really necessary ... in fact, in picking up Snow17, I combined them all into a single Init() -- if I recall correctly. Having an option allowing to overload that Init() function would be useful. Perhaps it could detect the shape of the object it's acting on to determine whether to go point or grid.

…

On Tue, Aug 29, 2023 at 12:41 PM GreyEvenson-NOAA ***@***.***> wrote: Thanks for having a look, @nmizukami <https://github.com/nmizukami>. I don't expect other ways of initialization so your suggestion makes good sense to me. I was aiming to replicate the mechanics of the noahowp_type initialization procedures <https://github.com/NOAA-OWP/noah-owp-modular/blob/30d0f53e8c14acc4ce74018e06ff7c9410ecc13c/src/RunModule.f90#L58> in the main branch, which has the Init and InitTransfer routines as public. Without being able to speak to Keith et al. about why they did things this way, I'd just guess that the approach offers better 'separation of concerns'. However, I also see the logic of your suggestion and I'd guess that it's generally preferable to reduce the number of public routines (?). It would be good to get @SnowHydrology <https://github.com/SnowHydrology>'s thoughts on this, when he's back in the office. I started working on this issue while he was gone so I need to circle back to make sure we're on the same page. — Reply to this email directly, view it on GitHub <#82 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIKARJ2GVZ3RA4ANL2IPELXXYZUBANCNFSM6AAAAAA35GRLPE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[nmizukami]

I think could use type bound generic procedure.

generic,   public  :: function => functionA, functionB
procedure, private :: functionA
procedure, private :: functionB

https://www.ibm.com/docs/en/xffbg/121.141?topic=binding-syntax-generic

[GreyREvenson]

I appreciate you taking the time, @andywood and @nmizukami. I'll work to move the InitTransfer calls to be within the scope of the Init calls and also look into overloading and specifying generic procedures. Talks soon. Thanks again!

[GreyREvenson]

Good morning @nmizukami and @andywood. Can one or both of you look over these revisions, if you have the time?

I moved calls to the InitTransfer subroutines within the scope of the Init subroutines per your suggestion. However, I elected not to choose between using the default values and the gridded values. Thus, I did not implement the generic procedure specification. My thinking was that we'd still always want to call InitDefault -- which is setting all variables to huge() -- to catch arithmetic problems. Then, we can set constants via a call to InitTransfer. Then, the remaining variables will be set via calls to the InTransfer subroutines (e.g., DomainVarInTransfer). If a variable is not set via calls to InitTransfer and the InTransfer (e.g., DomainVarInTransfer) subroutines, then InitDefault will help us catch resultant problems.

[nmizukami]

I think looks good to me. just one question I added (to make sure I got bigger picture how this works)

[nmizukami]

I think I understand what have been done here, but question is if you want to run at a point, do you still have to provide everything in the same format (1x1 grid)?

[GreyREvenson]

@nmizukami I believe you're correct, if I correctly understood your question. The code base - both before and after this PR - assumes that the model user provides gridded inputs (for land use, soils, etc).

That said, maybe we should plan to read-in default values from namelist.input? Read-in values could be used to parameterize a 1 x 1 grid. Then we could implement the sorts of checks that you and Andy were mentioning. Is this or similar functionality something that you believe we should implement?

I'm realizing now that we could possibly improve upon the naming of the InitDefault subroutines. As they are currently implemented, they are responsible for setting variables to huge() to help catch arithmetic errors. They do not set variables to 'default' values in the manner of "use this value if no gridded values were given". Are my concerns regarding the naming of the subroutines valid and do you think we should rename?

[GreyREvenson]

@nmizukami I'll go ahead and merge this. I'd be interesting in talking more if you think we should add some additional functionality - e.g., allowing use of values specified in namelist.input. I think we could make it happen, if we decide to do it. I appreciate you reviewing this!

[andywood]

@GreyEvenson-NOAA , one consideration is to keep 'namelist' as slim as possible, and geared toward inputs that are often changed and important to expose to users. Hardwiring defaults seems fine, as once a reasonable default is specified it will rarely be changed by a user; if they do, it's no longer a default. Bulky namelists are no fun ... you have to read past everything that's not relevant to pick out the few things worth updating (either machine or human).

Also, I'd advocate for InitTransfer to assign defaults as well, versus having a separate routine. Anything not init. by transfer gets the default, so no variables are assigned twice within a split second (ie, once as default and then as transfer). And that's one less subroutine to carry.

The SUMMA code, btw, has a good lexicon of type-based default values, eg integerMissing, floatMissing, doubleMissing ... that can be defined in one place in case there's a need to change. We opted for huge() because it was recognizable when debugging, but another standard would be fine too.

[nmizukami]

Hi Grey, Yes, I am happy to join further discussion. My thoughts (from user's standpoint) are the code can run with 2d grid (x and y like just lat/lon, or curvilinear with projected coordinate), point, or vector (i.e., HRU configuration). I think the models I knows - original noah-mp, clm, etc. use allocatable 2D arrays to hold the data (3D for soil layers etc.). If a user uses grid configuration, the code uses 2D array with nx and ny size allocated. if a user uses a victor, nx x 1 array, and 1 x 1 array (still allocatable array, not scalar) for a point run. I expect that there are lat and lon 2D array to tell center coordinates, and would be necessary to have ID array (especially if user runs with vector). The user is expected to prepare input data accordingly.