ribasim_nl Model class - Githubissues

DanielTollenaar commented 3 months ago

Hi @visr and @evetion,

My thought is to build a ribasim_nl.Model class inhereting all specs from ribasim.Model and add some extra features. I have made a start in this branch; now the results of a model can be read in a DataFrame directly by running the code below.

I am curious to your thought on this, if this is a proper approach or if there are better ways to achieve this.

I think this can benefit the ribasim-repo as I can imagine some code could be copied to the ribasim-repository later, e.g. model.merge(other_model), model.clip_by_polygon(polygon:shapely.geometry.Polygon).

Other methods probably will be NL-specific (e.g. forcing specific meta_columns at writing as mentioned in #68, init a model from the CloudStorage, etc).

from ribasim_nl import CloudStorage, Model

cloud = CloudStorage()
waterbeheerder = "Rijkswaterstaat"

ribasim_toml = cloud.joinpath("Rijkswaterstaat", "modellen", "hws_2024_4_4", "hws.toml")
model = Model.read(ribasim_toml)

df = model.basin_results.df

# %% cloud later be later model.basin_results.get_series(node_id=63)
df[df["node_id"] == 63].plot()

The start I've made is here: https://github.com/Deltares/Ribasim-NL/blob/ribasim_nl.Model/src/ribasim_nl/ribasim_nl/model.py

evetion commented 3 months ago

Hi @DanielTollenaar, nice to read about the ideas for Ribasim NL. My two cents:

I think Model as the class name can be confusing, especially if one works in both code bases. Something like NLModel (whether aliased at import or not) seems a better choice to me.
On inheritance is the age-old discussion on "Composition over inheritance". For Ribasim it made sense for some things (e.g. SpatialTable inheriting from Table), and everything inheriting from Pydantic BaseModel so validation is triggered. I can imagine that injecting some extra metadata on write calls makes sense in this way, but I would advise to just use composition. That would make it easier if you would ever relate to other classes (like some Dutch (meta)data?), and it wouldn't (possible subtly) break down when we change the implementation of a method that is shadowed here. Besides, even with composition it is straightforward to port functionality to Ribasim if/when needed.

DanielTollenaar commented 3 months ago

I think Model as the class name can be confusing, especially if one works in both code bases. Something like NLModel (whether aliased at import or not) seems a better choice to me.

I've considered NLModel first, but I deliberately changed it to Model as it would be super-easy to switch within existing building-scripts by chaning the import.

On inheritance is the age-old discussion on "Composition over inheritance".

Inheritance I only forsee for the Model-class, "extra gimmics" should be composed to this Model-class follow your lead. @visr In that sense, I woul normally expect to retrieve basin results as df by model.results.basin.df, but model.results seems to be reserved for result-settings rather than actual results, is that correct?

visr commented 3 months ago

model.results seems to be reserved for result-settings

Yes that's right. We don't really have any code for dealing with results other than model.to_xugrid(), see here:

https://github.com/Deltares/Ribasim/blob/72923fe334ab6b1651ac8c5b9e5164845ded1f26/python/ribasim/ribasim/model.py#L453-L473

Some API for results (if present) would be nice. Not sure if something like model.results.basin.df can be combined with the TOML results section.

visr commented 2 months ago

Will be part of https://github.com/Deltares/Ribasim-NL/tree/hws_2024.5

Deltares / Ribasim-NL

ribasim_nl Model class #91