search

NGEET / fates

repository for the Functionally Assembled Terrestrial Ecosystem Simulator (FATES)
Other
100 stars 92 forks source link

Migrate Constants from Modules like EDTypesMod to FatesConstantsMod #585

Open rgknox opened 5 years ago

rgknox commented 5 years ago

EDTypesMod is acting as a holding space for constants. Many of these constants are useful in a large number of contexts, such as defining flags like "leaf_on", and should be moved to FatesConstantsMod.

Some of these constants are specific to the cohort, patch and site level variables (like scratch space sizes), and perhaps its a good place to keep them.

FatesConstantsMod is a good place for constants to be defined because it has no dependencies, and therefore creates no circularities.

See also: #579, and re-label leaf_on and leaf_off flags in AllometricCarbonMod.F90.

rgknox commented 4 years ago

Circling back to this issue, I'm rethinking that it might be better to keep parameters associated with certain processes, more closely associated with the module for that process. For instance, "leaf_on" flags are phenology processes, and since we don't have a dedicated phenology module, or one that holds their data structures, then EDTypes seems fine.

These are sptifire specific parameters that could, and I would argue a good home is in fire/SFMainMod.F90 or fire/SFParamsMod.F90 https://github.com/NGEET/fates/blob/master/main/EDTypesMod.F90#L143-L148

glemieux commented 4 years ago

I agree with your latter thought; keeping constants and parameters associated with a particular module with that code sounds good to me. I imagine it would help someone not versed in the code based better understand the relationships between parameters and their associated modules.

ekluzek commented 4 years ago

It is a good goal to have data as local as possible. So if it's only needed in one subroutine, make it local there. If only one module, local there. And then make it protected if it's only set in one place, but used in others. So something that's specific only to spitfire would be good to just be local to spitfire.

But, of course many constants are useful almost everywhere, so they do need to be global.

jkshuman commented 4 years ago

@rgknox @ekluzek I think this is a good idea to move these to the spitfire module. can look through the list and confirm that these are only needed locally.