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

Update to newest Delft version and setup impacted population #406

Merged
merged 50 commits into from
Dec 12, 2024

Conversation

Santonia27
Copy link
Collaborator

@Santonia27 Santonia27 commented Oct 30, 2024

  1. update all headers to align with the newest Delft Fiat version
  2. add function to setup_impacted_population
  3. add clipping of exposure data to hazard if used setup_hazard. In the future will be added to delft fiat to have some tolerance.

Santonia27 and others added 12 commits October 21, 2024 16:55
* make variable names compatible with new Delft Fiat version

* add gfh attribute to setup exposure buildings

* update primary and secondary classification

* add docstring, add ground elevation unit, fix naming structure content

* setup populatoin remove geometry

* remove capitalized

* update gis

* uncomment zonal means
@savente93
Copy link
Contributor

Could you resolve the merge conflict? I'll review after. thanks

Comment on lines 89 to 90
"fn_damages_structure",
"max_damages_structure",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dalmijn @Santonia27 I get the error below in Delft-FIAT 0.20 and think this can be solved by changing damages in these lines with damage.

2024-11-27 15:41:40 ERROR For type: 'damage' no matching columns were found for fn_damage_* and max_damage_* columns.

Suggested change
"fn_damages_structure",
"max_damages_structure",
"fn_damage_structure",
"max_damage_structure",

self.new_ground_floor_height(config_yaml)
elif "Additional Attributes" in item:
self.new_additional_attributes(config_yaml)
elif "Ground Elevation" in item:
elif "ground_elevtn" in item:
self.new_ground_elevation(config_yaml)
elif "Max Potential Damages" in item:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this used and why does it have spaces?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its used in delft dashboard.
What do you mean with spaces?

@@ -221,6 +223,20 @@ def join_spatial_data(
right_gdf.at[index, 'geometry'] = largest_polygon
assert len(right_gdf.geom_type.unique()) == 1

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this part is repeated 3 times

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh oops i think i accidently copied the blocks

@@ -17,7 +17,7 @@ def get_max_potential_damage_roads(
roads = gpd.GeoDataFrame(
{
"lanes": pd.to_numeric(roads["lanes"], errors="coerce"),
"segment_length": roads.length,
"Segment Length": roads.length,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the idea that we only use the snake case for the mandatory FIAT columns?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since it s not used in fiat i didnt update it but I changed it now to snakecase also

Copy link
Collaborator

@panosatha panosatha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some minor comments. Please do a quick check on them and then I think we are ready to merge!

Copy link
Collaborator

@panosatha panosatha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Santonia27 Santonia27 merged commit 18a2e66 into main Dec 12, 2024
7 checks passed
@Santonia27 Santonia27 deleted the setup_impacted_population branch December 12, 2024 10:57
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.

4 participants