-
Notifications
You must be signed in to change notification settings - Fork 2
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
298 add attributes for aggregation areas and spatial joins #316
298 add attributes for aggregation areas and spatial joins #316
Conversation
…ins' of https://github.com/Deltares/hydromt_fiat into 298-add-attributes-for-aggregation-areas-and-spatial-joins
…298-add-attributes-for-aggregation-areas-and-spatial-joins
…l_attributes method. Spatial joins information are save and read with the FIAT model
…298-add-attributes-for-aggregation-areas-and-spatial-joins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks nice! just a few minor comments
exposure_gdf: gpd.GeoDataFrame, | ||
aggregation_area_fn: Union[List[str], List[Path], List[gpd.GeoDataFrame]], | ||
areas: Union[List[str], List[Path], List[gpd.GeoDataFrame]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why rename here to areas and everywhere else agregation_area_fn? Maybe it should be the same everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it back to aggregation_area_fn, but to be honest we call it aggregation areas but this could be anything, right? It could be any continues value maps (like SVI) etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's true, we were thinking about changing the name everywhere some months ago but then it got lost somehow.
so we could agree on a name and replace it all!
predicate="intersects", | ||
how="left", | ||
) | ||
|
||
# If we want to keep only used area we filter based on the intersections else we provide the whole dataframe | ||
if not keep_all: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we connect this option then also to the frontend with a checkbox button?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is a good idea. Let's leave as is for now and make an issue for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
|
||
exposure_gdf = join_exposure_aggregation_multiple_areas(exposure_gdf, aggregation_area_fn, attribute_names, label_names) | ||
exposure_gdf, areas_gdf = spatial_joins(exposure_gdf, aggregation_area_fn, attribute_names, label_names, keep_all) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
somehow keep_all is False here by default and also passed that way into the next function,
Shouldnt it be True by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My prespective is that we should only save the parts that are used so we avoid saving huge files. Imagine you have a file that covers a whole county and then you want a FIAT model for a town. Wouldn't it make sense to keep only that part?
Refactoring to allow for saving connections for the spatial joins (aggregation areas or additional attributes) that are performed during model build up: