#155 258 145 issues with main #215
Conversation
Update settings file and git ignore file
Update gitignore
Changing .gpkg files
Fixing integration hazard test, removing use on data catalog
…files Remove path to area of interest and define it as geojson in the test files
Implementation of setup ground elevation function including test
Data produced in the runs, this data must be included in the .gitignore file
Change gitignore to avoid conflicts
Update gitignore
update gitignore
Update git ignore
…-ground-elevation-to-the-buildings_v2 #155 add function to assign the ground elevation to the buildings v2
…n in new developed areas Adding changes to update maximum potential damage and ground elevation in new developed areas
Changes to merges branch with #145
Update gitignore to merge branch with #145
…nt-areas_v2 Adding ground elevation from a DEM to new development areas and updating maximun potential damage with input user data (points and polygons)
Change ground elevation test data
Data required for tests
Update test data
…amages-with-user-input-data #155-258-145-issues-with-main-updated
…iting function Integrate ground elevation from a digital elevation model into the writing function
|
I have in the branch all the outputs of my tests, for one reason I couldn't add them in git ignore file without creating conflicting the .gitignore of the main branch |
envs/hydromt-fiat-dev.yml
Outdated
There was a problem hiding this comment.
Please add xarray-spatial to the dependency list and also to the pyproject.toml
There was a problem hiding this comment.
I included them in the environment hydromt-fiat-dev.yml file, and the project.toml file
| "vulnerability_and_exposure_NSI": { | ||
| "data_catalogue": DATADIR / "hydromt_fiat_catalog_USA.yml", | ||
| "dir": "test_vulnerability_and_exposure_NSI", | ||
| "configuration": { | ||
| "setup_global_settings": {"crs": "epsg:4326"}, | ||
| "setup_output": { | ||
| "output_dir": "output", | ||
| "output_csv_name": "output.csv", | ||
| "output_vector_name": "spatial.gpkg", | ||
| }, | ||
| "setup_vulnerability": { | ||
| "vulnerability_fn": "default_vulnerability_curves", | ||
| "vulnerability_identifiers_and_linking_fn": "default_hazus_iwr_linking", | ||
| "functions_mean": "default", | ||
| "functions_max": ["AGR1"], | ||
| "unit": "feet", | ||
| "step_size": 0.1, | ||
| }, | ||
| "setup_exposure_buildings": { | ||
| "asset_locations": "NSI", | ||
| "occupancy_type": "NSI", | ||
| "max_potential_damage": "NSI", | ||
| "damage_types": ["structure", "content"], | ||
| "ground_floor_height": "NSI", | ||
| "unit": "ft", | ||
| }, | ||
| }, | ||
| "region": _region, | ||
| }, |
There was a problem hiding this comment.
This test is a duplicate of the test_vulnerability_exposure_NSI and can be removed
| "vulnerability_and_exposure_NSI": { | |
| "data_catalogue": DATADIR / "hydromt_fiat_catalog_USA.yml", | |
| "dir": "test_vulnerability_and_exposure_NSI", | |
| "configuration": { | |
| "setup_global_settings": {"crs": "epsg:4326"}, | |
| "setup_output": { | |
| "output_dir": "output", | |
| "output_csv_name": "output.csv", | |
| "output_vector_name": "spatial.gpkg", | |
| }, | |
| "setup_vulnerability": { | |
| "vulnerability_fn": "default_vulnerability_curves", | |
| "vulnerability_identifiers_and_linking_fn": "default_hazus_iwr_linking", | |
| "functions_mean": "default", | |
| "functions_max": ["AGR1"], | |
| "unit": "feet", | |
| "step_size": 0.1, | |
| }, | |
| "setup_exposure_buildings": { | |
| "asset_locations": "NSI", | |
| "occupancy_type": "NSI", | |
| "max_potential_damage": "NSI", | |
| "damage_types": ["structure", "content"], | |
| "ground_floor_height": "NSI", | |
| "unit": "ft", | |
| }, | |
| }, | |
| "region": _region, | |
| }, |
There was a problem hiding this comment.
I removed it and changed the name of the test with test_vulnerability_expousure_with_user_dem_data.py
| # Check if the exposure data exists | ||
| assert root.joinpath("exposure", "buildings.gpkg").exists() | ||
| assert root.joinpath("exposure", "exposure.csv").exists() | ||
| assert root.joinpath("exposure", "region.gpkg").exists() |
There was a problem hiding this comment.
Could you add assert statements that check for example if the Ground Elevation column is there and it is not Null? Perhaps some values are null and that is okay because the DEM does not reach there, but you have to check that manually first as well.
There was a problem hiding this comment.
I included two new asserts to evaluate if the column has no null values, and that the values are within a maximum and minimum values of the raster file
There was a problem hiding this comment.
Very nice that you added tests for all of the variations!!
| fm.set_root(_cases[case]["new_root"]) | ||
| fm.write() | ||
|
|
||
| unique_ge_new = fm.exposure.exposure_db["Ground Elevation"].unique() |
There was a problem hiding this comment.
Is this the most logical test to do? Maybe good to think more about how to assess the Ground Elevation column is filled. Maybe it cannot be checked by values but that all values are filled with a reasonable number maybe is good?
There was a problem hiding this comment.
I included the same asserts than test_setup_exposure_buildings_with_dem_data.py test
tests/test_integrations_hazard.py
Outdated
There was a problem hiding this comment.
I like these improvement!
| "dir": "fiat_model", | ||
| "new_root": EXAMPLEDIR / "test_update_max_potential_damage_points", | ||
| "data_catalog": DATADIR / "hydromt_fiat_catalog_USA.yml", | ||
| "max_potential_damage_file": EXAMPLEDIR |
There was a problem hiding this comment.
You're using the same geometry files for this test as for the update_ground_floor_height test. Is that the idea? Maybe it does not matter for the testing itself but the naming is confusing.
There was a problem hiding this comment.
I modified the files names to make them clearer
There was a problem hiding this comment.
Did you also check the outputs by hand? That the right values are taking over by the correct building points?
There was a problem hiding this comment.
I miss this components, should I check it by hand or try to make an assertion out of it?
There was a problem hiding this comment.
I manully checked the objects within the polygons (e.g. 574488798 object) and it correctly change the value.
| # This function was developed by Willem https://github.com/interTwin-eu/DT-flood/blob/DemonstrationNotebooks/Notebooks/SetupFIAT.ipynb | ||
| #TODO: Find equivalent functions in hydromt | ||
| # Read in the DEM | ||
| dem = rasterio.open(ground_elevation) |
There was a problem hiding this comment.
Can you put the code under if ground_elevation: for getting the ground elevation and adding it to the exposure objects in a separate function in gis.py?
There was a problem hiding this comment.
It is implemented now in gis.py
Update xarray-spatial in environment and project .toml
…tial damage from users data Improvement of tests for assigning ground elevation and maximun potential damage from users data
… with user data test Updating ground elevation test, and updating maximum potential damage with user data test
Update test vulnerability exposure with dem data
…dem() function remove comented lines that were remplazed with ground_elevation_from_dem() function
The new pull request integrates the calculation of ground elevation from a digital elevation file (.tiff), the tool is flexible and calculates Ground Elevation just in case a dem file is provided. Currently .setup_buildings_from_single_source() is the only option that has been tested. The function was also integrated in .setup_buildings_from_multiple_sources() but I require test data that triggers this function. In .setup_buildings_from_single_source() I have realized that it is difficult to access the geometries (which is required in setup_ground_elevation), even after running .set_exposure_geoms() which gives me the impression that this function is not working correctly, because it creates a list of geometries and not a geopandas dataframe (but maybe that is the way this function works). I accessed to the geometries maybe in to the best optimal way so is best to double check if that is the best alternative.