Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return value and typing documentation discrepancies #137

Open
SorooshMani-NOAA opened this issue Apr 27, 2022 · 7 comments
Open

Return value and typing documentation discrepancies #137

SorooshMani-NOAA opened this issue Apr 27, 2022 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@SorooshMani-NOAA
Copy link
Collaborator

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:

@property
def adcircpy_forcing(self) -> Forcing:

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 SorooshMani-NOAA added the enhancement New feature or request label Apr 27, 2022
@SorooshMani-NOAA
Copy link
Collaborator Author

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
Copy link

ghost commented Apr 27, 2022

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

@SorooshMani-NOAA
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@ghost
Copy link

ghost commented Apr 29, 2022

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

@SorooshMani-NOAA
Copy link
Collaborator Author

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant