Fiat adapter refactoring #433
Conversation
…into fiat_adapter_refactoring
…into fiat_adapter_refactoring
…into fiat_adapter_refactoring
| @@ -42,17 +41,17 @@ def __init__(self, database: IDatabase): | |||
| def get_aggregation_areas(self) -> dict: | |||
| """Get a list of the aggregation areas that are provided in the site configuration. | |||
|
|
|||
| These are expected to much the ones in the FIAT model. | |||
| These are expected to much the ones in the direct impacts model. | |||
| @abstractmethod | ||
| def set_hazard(self, hazard) -> None: | ||
| """ | ||
| Set the hazard data for the model. | ||
|
|
||
| Args: | ||
| hazard (Hazard): The hazard object containing the necessary information. | ||
|
|
||
| Returns | ||
| ------- | ||
| None | ||
| """ | ||
| ... |
There was a problem hiding this comment.
I dont think we should directly hard-couple the hazard to the direct impacts like this.
I think we had a very short discussion about this and decided we were going to put a class HazardOutput in between the Hazard models and the DirectImact models to completely decouple them. Where HazardOutput can have multiple formats.
That way any hazard model can implement functions to output its data to a HazardOutput and any DirectImacts model can then implement functions to read HazardOutput.
Implementing that is not the scope of this PR, so we can still merge this one, but we should keep this in mind. This is very related to the discussion with Dirk and Hessel as well, so good that we are looking at this now!
| def rel_to_abs_path(path): | ||
| if path: | ||
| path_abs = self.database_static_path.joinpath(path) | ||
| return path_abs |
There was a problem hiding this comment.
Path(..).resolve() does this. Not that it really matters, just good practice to use builtin libraries if they are available!
There was a problem hiding this comment.
The idea of generalizing the DirectImacts adapter is great! However, we need to give some more consideration to what will be in the Interface and what should be in the ConcreteAdapter classes. The current implementation is very much based on fiat (obviously), which is not really neccesary as that functionality can be in the FiatAdapter itself.
This AdapterInterface is something that all DI adapters have in common and should in my opinion only contain very generic functions. Which are then implemented for the each model and call specific functions implemented for that model.
preprocess() # setup model from template + read_hazard_output + x * add_measure()
run()
postprocess()
has_run_check()
Possibly:
add_measure(measure: IMeasure)
# Implement this function to handle all measures that make sense for that DirectImpact model, and do nothing for measures that dont affect this model. (think of raising buildings for RA2CE for example)
read_hazard_output(output: IHazardOutput)
write_output(dst: Path)
With the goal of being able to treat all Hazards/Impacts agnostically like this: (where hazards & direct_impacts are lists of implemented Adapters)
def run_scenario()
for hazard in hazards:
hazard.preprocess()
hazard.run()
hazard.postprocess()
for direct_impact in direct_impacts:
direct_impact.preprocess()
direct_impact.run()
direct_impact.postprocess()
There was a problem hiding this comment.
See comment on direct_impacts_adapter.py.
This adapter class needs to implement all @abtractfunctions in the DirectImpactsAdapter Interface.
There was a problem hiding this comment.
Might be a good idea to have a look at how to do the dbs_classes refactor from Daley to the benefit class as well. It seems unnecessary to store the 3 database paths in a benefit object. Again, not the scope of this PR, but good to keep in mind.
No description provided.