refactor(adapters): Add adapter interfaces and refactor SficnsAdapter to use them. Also refactor events to use forcings #473
Conversation
move sfincs specific code from hazard.py to SfincsAdapter. implement IForcing + child classes
… functions clearer
create interfaces for the adapters create interface for mediator (the one im not sure of, but we need some kind of management class to handle the adapters) create HazardOutput enum, needs more work/thought
There was a problem hiding this comment.
I think this is on a really good track. Some things might still get messy, for example, when event.water_level is set to from_model, we need to add wind and pressure to the offshore model of it's an historic_offshore event.
Overall, I really like it. I made some comments in the SFINCS adapter, where I think the existing functions should go in the new hazard class logic. And I also made some overall comments. Perhaps not all of them make sense to you. Then we should catch up.
Small note: Where are we plotting the timeseries of event forcings (that are shown in the event pop up window)?
…into refactor/hazard-adapter-and-events
implement eventfactory and forcingfactory
flood_adapt/object_model/hazard/event/forcing/forcing_factory.py
Outdated
Show resolved
Hide resolved
flood_adapt/object_model/hazard/event/forcing/forcing_factory.py
Outdated
Show resolved
Hide resolved
flood_adapt/object_model/hazard/event/forcing/forcing_factory.py
Outdated
Show resolved
Hide resolved
start on sfincsadapter tests
There was a problem hiding this comment.
Hi,
I reviewed it all (just skipped the tests). My biggest comments are:
- Keep any model processing and execution out of the event classes because events are agnostic of SLR.
- How we deal with event sets is messy. Let's discuss
- Keep the postprocessing (rp_floodmaps) outside of the event class as well. This needs to remain in the hazard class (for now, we can move later, but let's not make this PR even bigger). This is only applicable to event sets, which can be recognized by MODE=="risk"
| df.index = pd.to_datetime(df.index, format=DEFAULT_DATETIME_FORMAT) | ||
| return df | ||
|
|
||
| def to_dataframe( |
There was a problem hiding this comment.
No, timeseries do not need the same timestep. All forcings just need to cover the entire simulation period.
…eters allow synthetic discharge for synthetic event
renamed constant timeseries to block for clarity
removed constants [TIDAL_PERIOD, DEFAULT_DATETIME_FORMAT] from the code
…nly data, the adapter uses the data
cleanup forcing interface
moved plotting functions from event class to separate file
… can go unnoticed (#619)
|
Also sorry, I thought that you could submit multiple scopes, but it seems you can't yet. So we'll have to choose one for adapter or object-model. I would suggest object-model since that seems the more important topic to me, but I think you could make an argument for either. |
LuukBlom
changed the title
@GundulaW : I didnt get the refactor working yet (its getting quite big but im not sure how to nicely split it up into more PRs).
I have not removed the majority of old code, so the repo might seem a bit busy (and I didnt do imports yet so pretty much all that uses the Hazard class is broken, including all tests).
The low-level intelligence in the Forcing classes is not yet implemented, as I'd like to get your take on it before spending too much time.
I'd love some feedback on the more domain related functionality (the SfincsAdapter is the main one, but also the Adapter interfaces, Forcings, EventModels & Events). for the SfincsAdapter, I tried to move the code from hazard.py to the adapter with as little chages as possible, but it is a lot of code so I probably introduced some bugs.
I added the Timeseries class I've told you about here as well, even though its less nice than I wanted it to be.. It can create any 1D timeseries from a SyntheticTimeseriesModel (with attrs = shapetype, shape_duration, peak_time, peak_value/cumulative), interpolate the data to a timestep, then write to a pdDataFrame while taking into account the start time & end time of an event, before adding it to a dataframe. I think it simplifies the use of Timeseries data for other parts of the code, but I also think its not perfect as it can only handle 1D Timeseries (Synthetic or csv). So wind(2D with speed and angle) is already more difficult, not to mention spatially varying gridded timeseries.