NOAA-OWP / snow17

Other
4 stars 10 forks source link

non-unique variable / function names limits combination options for standalone use #29

Closed andywood closed 8 months ago

andywood commented 1 year ago

The snow17 & sac-sma codes were split apart from the same source codebase when refactoring to work with BMI and Nextgen. Snow17 was done first and provided a template for sac-sma, and in the end a lot of the same generic data structure names, function names, etc. were used (eg modelVar).

There's interest from Reclamation in building a standalone driver for the OWP versions that runs snow17, sac-sma, adds back UH routing, lagK and some other standard NWS routines. A standalone NWS hydro modeling suite is a useful benchmark in the hydrology world. Of course the Nextgen framework could do this but it comes with a lot of overhead that may be superfluous to an application.

It might be useful to change generic names into model-specific names (eg forcings -> snow17_forcings, initialize -> snow17_initialize) so that a number of models could be easily combined and run w/ one BMI-based driver.

SnowHydrology commented 1 year ago

@andywood I would recommend against doing this in the main branch here. BMI is specifically designed to use generic function names so that a centralized framework can call these functions without relying on redundant, model-specific code. This genericness is particularly useful when the models are built as shared libraries, meaning a framework can access their functions without having to know anything about their names. For example, you wouldn't want a framework to run the following pseudocode:

model.snow17_initialize()
model.sacsma_initialize()

Any time you added a model, you would have to manually edit the initialize, get/set, update, etc. calls.

That being said, there may be necessary concessions when including other NWS routines in a framework outside of NextGen. However, a counterpoint would be: is it easier to adapt these other routines to NextGen than it is to create a standalone framework that some of NextGen's functionality?

Tagging @hellkite500 and @mattw-nws

mattw-nws commented 1 year ago

A standalone NWS hydro modeling suite is a useful benchmark in the hydrology world. Of course the Nextgen framework could do this but it comes with a lot of overhead that may be superfluous to an application.

It won't surprise you at all to hear me say I think running these models in the NextGen framework is exactly what an NWS hydro modeling suite would look like... I may not understand the problem entirely but I'd be interested in hearing what superfluous overhead may be a concern and if there's anything we can do about that... e.g. we don't have to do all the hydrofabric stuff like we're doing for NWM-targeted work, so there may be non-obvious things that can be cut or bypassed.

I'm not sure what "adds back UH routing" means and that might be something more difficult in the current framework, but maybe not.

Generally, if I'm understanding correctly, I agree with Keith's comments above...it even bugs me a little that PET has essentially five models rolled into one module rather than being separate modules.. I wouldn't recommend doing something like what is being suggested.

Generally, also, the solution to this kind of thing is to bring "out" the common data structures and functions into a "library" of some form and just include that library in, say, a snow17 and sac_sma module... that library can just be a shared set of source files or a actual .so or .a file... then the modules can still be compiled separately and have their own functions in their own namespaces (e.g. initialize) but still reuse common code. And reuse of common data structures is definitely something I would encourage. Can assist or make further suggestions if you point me to more details.

andywood commented 1 year ago

Great feedback, Keith & Matt. On reflection and given your comments, I see there's a better way to do this than I suggested. I haven't taken time to go through the code and think about how this could work completely, though. I wanted to record the issue while it was in front of me so that if we have time to develop on it later, it's not forgotten.

SnowHydrology commented 8 months ago

Closing this issue as the suggested modifications won't be made per previous comments.