CTSM icon indicating copy to clipboard operation
CTSM copied to clipboard

Create helper function for interpMontlyVeg to improve readability

Open adrifoster opened this issue 2 years ago • 6 comments

Description of changes

In reading through clm_drv, I found this set of if/else statements to be a bit squirrelly. I think the logic of it could be improved for readability, and possibly we could move it to a helper function inside SatellitePhenologyMod.

Specific notes

I've created a WIP PR to discuss among ourselves. @ekluzek brings up that this could serve as a template for other similar sections of the code

Specific questions I still have:

  1. we have use_fates .and. use_fates_sp, but am I correct that use_fates_sp cannot be .true. unless use_fates is? So we really need only have use_fates_sp?
  2. Should the conditionals be function arguments or "uses"? I am leaning towards function arguments (surprise surprise... /s) because it tells the user exactly what is being evaluated in order to decide to run interpMonthlyVeg

Contributors other than yourself, if any: @ekluzek

CTSM Issues Fixed (include github issue #): None

Are answers expected to change (and if so in what way)? No

Any User Interface Changes (namelist or namelist defaults changes)? No

Testing performed, if any: None yet, will test after we discuss.

adrifoster avatar Aug 29 '23 16:08 adrifoster

In answer to your questions:

  1. we have use_fates .and. use_fates_sp, but am I correct that use_fates_sp cannot be .true. unless use_fates is? So we really need only have use_fates_sp?

That is a good point. It is true. I always feel like you have to check both, but we don't allow use_fates_sp to be on, unless use_fates is on. So you are correct that you only need to check use_fates_sp. The one reason I'm thinking of is that at the high level we only want the model to know about FATES on or off. Only in the guts of FATES do we want to have the list of specific FATES options.

That said use_fates_sp is also a high level control variable that is needed at high levels in CTSM. So both it and use_fates are needed at the high level. So I'm convincing myself that you are right we only need to check use_fates_sp.

  1. Should the conditionals be function arguments or "uses"? I am leaning towards function arguments (surprise surprise... /s) because it tells the user exactly what is being evaluated in order to decide to run interpMonthlyVeg

I agree function arguments give a clearer picture of the flow in the code. "uses" are global variables and if they can be avoided at the lower level it makes sense to me. For function arguments you can also declare them as intent IN or OUT, whereas "uses" imply IN/OUT, so it can improve the error checking in the code as well.

ekluzek avatar Jan 19 '24 18:01 ekluzek

@adrifoster this looks to me like a little thing that we could just bring in like it is? Do you agree? It looks like a simple bit-for-bit refactoring to improve the code.

ekluzek avatar Jan 19 '24 18:01 ekluzek

@ekluzek and I expected this PR to close/merge with #2334, but it still appears Open. That's convenient :-)

slevis-lmwg avatar Jan 25 '24 00:01 slevis-lmwg

See errors here.

samsrabin avatar Jan 25 '24 16:01 samsrabin

I will work on this to see what is going on. I believe this worked before, but it's possible something changed in the intervening time...

adrifoster avatar Jan 25 '24 16:01 adrifoster

But I'm pretty sure it has something to do with the logic of how this gets called in clm_driver vs. how it gets called in clm_initialize

adrifoster avatar Jan 25 '24 16:01 adrifoster