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

Stable version of the fiat_integrator #124

Merged
merged 384 commits into from
Oct 19, 2023
Merged

Stable version of the fiat_integrator #124

merged 384 commits into from
Oct 19, 2023

Conversation

frederique-hub
Copy link
Collaborator

@frederique-hub frederique-hub commented Aug 8, 2023

Done before requestion the review (for all except the setup_hazard function):

frederique-hub and others added 30 commits June 23, 2023 11:04
Modification of loadflood maps tool
Change hazard to make it compatible with sfincs inputs
Update test to read only maps without rp dimension
panosatha and others added 10 commits October 13, 2023 14:16
corrected raising of buildings when using datum reference
Update fiat_integrator with env_fix
Modifications made after Frederique's review. Hydromt sfincs functions are out of hydromt fiat now. Every dataset required for hydromt fiat can be done in advace with hydromt sfincs.
Makin hazard function explicit after Dirk review
Change of .rio for .raster hyndromt core function
Adding parameter geom when reading raster data. It requires that self.region is always available.
Removing test using sfincs_map.nc. They are not processed anymore through hydromt fiat.
@frederique-hub
Copy link
Collaborator Author

frederique-hub commented Oct 17, 2023

Great work FIAT team! I've checked fiat.py and generally the code is easy to read. The example in the docs is also very illustrative! And I like the census and osm api implementation. We will soon be able to support this through the DataCatalog as well.

I've left some comments in the code but my main comment / concern is about the exposure and vulnerability data: The exposure and vulnerabilty items seem to be saved in separate class properties (e.g. exposure.exposure_geoms) AND in the model geoms and tables properties. I wonder why this choice was made? This seems confusing to me and a potential memory / synchronising issue. If a user makes changes to the geoms and writes the model this will be overwritten by the original exposure in the the exposure class by the update_all method and the edits will be 'undone'. I would strongly suggest to save these items in one location only. I'm happy to discuss how this could be achieved.

Idea: make the geoms property dynamically linked to the ExposureVector class exposure_geoms property? (DATA IN THE EXPOSURE CLASS)

--> make a property in fiat.py instead of the update functions, use the 'super' functions (inherit from parent class) to update the geoms property within HydroMT core)

Another idea: don't store data in the class properties but only the properties that define the data. Save the data in the hydroMT classes/properties. (MORE FUNCTIONS IN EXPOSURE)

Question: will the user work with HydroMT or also with the classes underneath?

After solving this we will merge the fiat_integrator to the main branch

@frederique-hub
Copy link
Collaborator Author

The issue of saving the exposure geoms and tables in multiple places is now solved by only saving them in the geoms and tables properties in the 'write' function of the FiatModel.

@frederique-hub frederique-hub merged commit 3dd3c38 into main Oct 19, 2023
1 check passed
@frederique-hub frederique-hub deleted the fiat_integrator branch November 10, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants