Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Move water fluxes back into waterflux_type
I had hoped that most variables computed by a given module could be owned by the type defined in that module - i.e., moving more towards "textbook" object-orientation. However, I realized that this won't work well for water fluxes due to the requirements of isotopes and other water tracers: We need multiple instances of each type that contains any fluxes for which we need separate instances for isotopes / tracers, but we don't want separate instances for all variables. Mariana Vertenstein and I considered a number of solutions to this problem, but in the end decided it's best to just move these water fluxes out of the science modules into waterflux_type, because other solutions we could think of had bigger downsides. I feel like there was some value in the idea of having the science modules own these fluxes, but not enough value to warrant the extra complexity it will cause in the face of water isotopes / tracers. I'd still like to keep some variables local to the science modules – especially variables that can be made private to those modules. But the fluxes were never handled in a completely (textbook) object-oriented fashion anyway, since they were accessed publicly by other modules, so I'm not too sad to move these public flux variables back out to the generic flux types. And while this has the disadvantage of losing the attachment of a variable to the module that computes that variable, it has some advantages, too, in terms of understanding the suite of water flux variables, and reducing the number of instances that need to get passed all around (e.g., we won't need to pass irrigation_inst to as many places). (See ESCOMP#395 (comment) for more details.) I'm even moving fluxes that could otherwise be private to the module in which they're set. This is partly because these private fluxes may at some point need multiple instances for isotopes / tracers (this is the case for the fluxes in GlacierSurfaceMassBalanceMod - the separate melt and freeze fluxes - which seem like they may need to be tracked separately for isotopes... actually, I could envision needing track the melt flux, qflx_glcice_melt, separately by layer for the sake of isotopes), and partly because I don't want to have to change the home of a variable just because some other module now needs it and so it now needs to be public. It's possible that not everything that is called a flux really needs to go in waterflux_type. I'm particularly thinking about partial fluxes - e.g., if we add foo + bar to get the full flux from state1 to state2, then it's possible that we only need to store that sum in waterflux_type, and could keep foo and bar in the science modules that set them. (Specific example: I'm wondering if qflx_sat_excess_surf and qflx_infl_excess should really be considered fluxes, or if these "partial fluxes" can continue to live in their defining science modules.) But I'm not sure about that (it may be that we'll need tracer versions of even these partial fluxes), and even if it's true, a downside would be that this could increase the volatility of where things are declared (e.g., if it used to be that foo was the only flux, and then we added bar later - would we then move foo out of waterflux_type to its defining science module?) So at least for now, I'll move these "partial fluxes" to waterflux_type; I can always move them back once I understand the needs for tracers / isotopes better. Another example of something that may not need to be in waterflux_type is qflx_glcice_dyn_water_flux_col - but it seems possible that we'll need separate isotope versions for this, so I'll move it for now, and we could move it back later once we understand what's needed for isotopes better. I'm leaving non-flux variables in the module that computes them, even if they're public - e.g., qinmax_col and fsat_col. I can see arguments either way for these. For now I'll keep them in place, partly because that's easier than moving them. But I can see a general principle: Fluxes should be contained in waterflux_type, but auxiliary variables (public or private) that are computed by a module should be owned by that module. One justification for this is: For non-water-flux variables that are only referenced by that module, it makes sense to make them owned by that module and private. And we don't want to have to move things around just because now someone else needs them. So where a variable lives should be based less on public/private and more on whether it's a water flux (which needs to be in the water flux type) or something else (which can follow "better" object-oriented design). For water fluxes computed by a routine, I considered passing them in directly as intent(out) arrays rather than passing the whole waterflux_type in, to make it more clear what the effect of the routine is. But the problem I see with passing computed fluxes directly as arrays is the potential for aliasing - i.e., if both waterflux_inst and waterflux_inst%foo are passed to a routine, the latter as an output, and then inside the routine, waterflux_inst%foo is used as an input, in addition to setting foo. A solution could be to pass everything as bare arrays, but this goes against what we have in place right now, and I don't love that idea because, if we have two polymorphic implementations, it makes it harder for them to have different input requirements. So for now I'm sticking with passing in the whole waterflux_type and documenting outputs in the associate block.
- Loading branch information
