Add secondary object type #413
Conversation
Santonia27
commented
Nov 7, 2024
•
Santonia27
commented
- add secondary Object type column if not existing and use Primary object type values (needed in the database builder)
- divided the units into: unit (model unit - default feet), gfh_unit, ground_elevation unit >> Now they can all be provided in the config file under setup_exposure_buildings
- added a conversion function for gfh and ground elevation. If these units != model unit they will be converted to the model unit
- updated the OSM download code. The code before seemed to divide the asset geometries into some weird shapes using the occupancy geometries
- added 'Multipolygon' to some if statements as I had data with only multipolygons. >> Maybe we need to discuss this as I have no experience with the geometries and I don't know if this will make a difference in the model output. If so we can remove it but then we need to think of a solution how to handle multipolygon data or we leave it to the user to clean it up fully before
…add_secondary_object_type
There was a problem hiding this comment.
I have made some comments on some things that I think need some improvements.
In general I think we should have units as arguments only in the setup functions that use external inputs. Then the units of the actual exposure vector object should be decided when the object is created.
hydromt_fiat/api/data_types.py
Outdated
| @@ -81,25 +80,28 @@ class ExposureBuildingsSettings(BaseModel): | |||
| occupancy_type: str | |||
| max_potential_damage: str | |||
| ground_floor_height: Union[str, float] | |||
| unit: Units | |||
| gfh_unit: Union[str, Units] = None | |||
| unit: Union[str, Units] | |||
There was a problem hiding this comment.
does it make sense to have the Union here? Isn't the main purpose of the Units enumerator to only allow these values?
There was a problem hiding this comment.
This still uses the Union. Should change that as well, right?
| @@ -82,6 +82,8 @@ def __init__( | |||
| unit: str = Units.feet.value, | |||
| country: str = None, | |||
There was a problem hiding this comment.
the country attribute does not seem to be used now
There was a problem hiding this comment.
this is still not used. Should we delete it?
@Santonia27 country is used for OSM and JRC setup
There was a problem hiding this comment.
this is the init method that country is not used. It is an argument on the respective methods. So either we need to delete it from here, or save it in self. here and not have it as an argument in the other methods.
| @@ -323,11 +331,13 @@ def setup_buildings_from_multiple_sources( | |||
| max_potential_damage: Union[str, Path], | |||
| ground_floor_height: Union[int, float, str, Path, None], | |||
| extraction_method: str, | |||
| unit: str, | |||
There was a problem hiding this comment.
it seems that "unit" is not used anymore
|
|
||
| # Remove the objects that do not have a Primary Object Type, that were not | ||
| # overlapping with the land use map, or that had a land use type of 'nan'. | ||
| if "Primary Object Type" in gdf.columns: | ||
| nr_without_primary_object = len( | ||
| gdf.loc[gdf["Primary Object Type"].isna()].index | ||
| ) + len(gdf.loc[gdf["Primary Object Type"] != ""].index) | ||
| ) + len(gdf.loc[gdf["Primary Object Type"] == ""].index) |
There was a problem hiding this comment.
maybe it is better to actually change all the "" cells to None, so from this point onwards you can use only a single filtering with .isna() isntead of having to do both?
hydromt_fiat/fiat.py
Outdated
| @@ -398,10 +407,13 @@ def setup_exposure_buildings( | |||
| max_potential_damage, | |||
| ground_floor_height, | |||
| extraction_method, | |||
| unit, | |||
There was a problem hiding this comment.
here I think the unit should not be an input arguments (see above my comment)
hydromt_fiat/api/data_types.py
Outdated
| @@ -81,25 +80,28 @@ class ExposureBuildingsSettings(BaseModel): | |||
| occupancy_type: str | |||
| max_potential_damage: str | |||
| ground_floor_height: Union[str, float] | |||
| unit: Units | |||
| gfh_unit: Union[str, Units] = None | |||
| unit: Union[str, Units] | |||
There was a problem hiding this comment.
This still uses the Union. Should change that as well, right?
hydromt_fiat/fiat.py
Outdated
| @@ -313,13 +312,15 @@ def setup_exposure_buildings( | |||
| max_potential_damage: Union[str, Path], | |||
| ground_floor_height: Union[int, float, str, Path, None], | |||
| unit: str, | |||
There was a problem hiding this comment.
this should be Units = None, and maybe put it somewhere on top, because now it is in between the ground_floor_height and the gfh_unit which is kind of confusing.
Moreover maybe better to rename it to length_unit as we discussed?
There was a problem hiding this comment.
If I make it None I cannot place it above a parameter without default input. Also I'm wondering if the model unit maybe should not be optional? It seems important to set the base unit by the user and not a default that the user may not be aware of? What's your opinion on that?
| @@ -386,7 +394,7 @@ def setup_exposure_buildings( | |||
| asset_locations, | |||
| ground_floor_height, | |||
| extraction_method, | |||
| ground_elevation_file=ground_elevation_file, | |||
| ground_elevation=ground_elevation, | |||
| ) | |||
There was a problem hiding this comment.
why do we not use the units in this method?
There was a problem hiding this comment.
its not used in the function, same as in the function below (took it out there)
| @@ -323,11 +331,13 @@ def setup_buildings_from_multiple_sources( | |||
| max_potential_damage: Union[str, Path], | |||
| ground_floor_height: Union[int, float, str, Path, None], | |||
| extraction_method: str, | |||
| unit: str, | |||
hydromt_fiat/fiat.py
Outdated
| @@ -398,10 +406,13 @@ def setup_exposure_buildings( | |||
| max_potential_damage, | |||
| ground_floor_height, | |||
| extraction_method, | |||
| unit, | |||
There was a problem hiding this comment.
unit should be delete from here and from the method
There was a problem hiding this comment.
its used in the function, why delete?
if hasattr(length_unit, "value"):
length_unit = length_unit.value
self.exposure = ExposureVector(
self.data_catalog,
self.logger,
self.region,
length_unit=length_unit,
damage_unit=damage_unit,
)
| @@ -82,6 +82,8 @@ def __init__( | |||
| unit: str = Units.feet.value, | |||
| country: str = None, | |||
There was a problem hiding this comment.
this is still not used. Should we delete it?
@Santonia27 country is used for OSM and JRC setup
| @@ -399,6 +411,9 @@ def setup_asset_locations(self, asset_locations: str) -> None: | |||
|
|
|||
| # Set the asset locations to the geometry variable (self.exposure_geoms) | |||
| # and set the geom name | |||
| if len(assets.columns) > 2: | |||
|
|
||
| # Remove the objects that do not have a Primary Object Type, that were not | ||
| # overlapping with the land use map, or that had a land use type of 'nan'. | ||
| if "Primary Object Type" in gdf.columns: | ||
| nr_without_primary_object = len( | ||
| gdf.loc[gdf["Primary Object Type"].isna()].index | ||
| ) + len(gdf.loc[gdf["Primary Object Type"] != ""].index) | ||
| ) + len(gdf.loc[gdf["Primary Object Type"] == ""].index) |
hydromt_fiat/fiat.py
Outdated
| @@ -398,10 +407,13 @@ def setup_exposure_buildings( | |||
| max_potential_damage, | |||
| ground_floor_height, | |||
| extraction_method, | |||
| unit, | |||
|
