Return value and typing documentation discrepancies

[SorooshMani-NOAA]

While working on adding PySCHISM support, I noticed that in both PySCHISM and ADCIRCPy some of the "model forcing" types are not actually derived from the base Forcing (in case of ADCIRCPy) or ModelForcing (in case of PySCHISM) types. One example of this is Tides type in both. In spite of this, the return values are assumed to be of Forcing type for ADCIRCPy configuration classes: https://github.com/noaa-ocs-modeling/CoupledModelDriver/blob/a97e5ec9ee44ed30cf8cb313b8418f4807f1ce3e/coupledmodeldriver/configure/forcings/base.py#L148-L149 How should this be addressed? This issue becomes more crucial when adding other models (e.g. PySCHISM in my case #136).

This issue discusses possible solutions for this issue

[SorooshMani-NOAA]

The basic issue is that ForcingJSON has to_adcircpy which should ideally return a single type - a superclass of all forcing types - is returning the superclass of only "some" of the forcing types; e.g. Tides is a Boundary Condition not a subclass of Forcing. While it can be debated that tidal boundary conditions are not forces physically or mathematically, still it would be good to have a single abstraction returned from the this method. Let's just call every constrain on the model a "force". The problem now becomes how to provide that abstraction. Should there be ADCIRCPyForcingAdaptor or PySCHISMForcingAdaptor class? @zacharyburnettNOAA how would you see the Enum work here? An alternative approach is to have a ADCIRCPyGenerator or PySCHISMGenerator objects that get these JSON configuration objects as input and generate solver specific outputs, instead of the *JSON objects all having to_<model> methods. This might require extensive refactoring.

[ghost]

that might be a more extensible and modular approach, instead of having to incorporate a to_<model> method for each model

[SorooshMani-NOAA]

Adding the "generator" classes would change the API a little bit which also requires a change in tests. Is that OK to proceed with it? I can create a new branch and try it and then switch back to pyschism effort

[SorooshMani-NOAA]

Actually I guess it might be better if instead of a single generator class, we have a generator for each forcing inside the model (e.g. coupledmodeldriver/generate/adcirc/forcings/*). Does that make sense?

[SorooshMani-NOAA]

If we use generators, we need to find a good way to take care of from_adcircpy and from_user_input methods too

[ghost]

sure thing! I think that would work with only a few changes

[SorooshMani-NOAA]

@zacharyburnettNOAA I'm still a little bit stuck here. The ideal solution for me looks like this: configure modules are base, then generate modules rely on the configure, and lastly client relies on both. Also I'd like configure not to have solver specific (SCHISM or ADCIRC) code. But I don't see how it's possible.

I can move the *.to_adcirc() and *.adcirc_forcing stuff under generate/adcirc; but then for the from_adcirc functionality I either need to add/keep adcircpy dependency in configure or instead move from_* to generate/adcirc as well; then I'll have circular dependency between configure and generate/adcirc. Alternatively the *JSON classes could be modified such that there's no from_adcircpy or to_adcircpy or adcircpy_forcing method or property, but rather the object needs to be passed to another object or function defined in generate/adcirc or generate/schism to go back and forth between JSON and Configuration realization of it.

What do you think?

For now instead of this refactoring, I'll focus on adding SCHISM. But let's continue discussing the approach to refactoring to make it ocean model agnostic!