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.

Sign up for GitHub

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

Jump to bottom

#318 new composite area aggregation #284

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

#318 new composite area aggregation #284

wants to merge 26 commits into from

Conversation

Santonia27
Copy link
Collaborator

No description provided.

Santonia27
Santonia27 commented Mar 6, 2024
View reviewed changes
examples/global_OSM_JRC.ipynb Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I played around the other day with this file and accidently pushed the changes but I changed everything back to the original state so I was hoping this would disappear from the PR

panosatha
panosatha reviewed Apr 25, 2024
View reviewed changes
hydromt_fiat/workflows/exposure_vector.py
@@ -1014,6 +1014,7 @@ def setup_new_composite_areas(
aggregation_area_fn: Union[List[str], List[Path], str, Path] = None,
attribute_names: Union[List[str], str] = None,
label_names: Union[List[str], str] = None,
new_composite_area: bool = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this used?

@panosatha
Copy link
Collaborator

Some of the issues I found:

  • Most important: the splitting based on the aggregation areas is performed after the max potential value has been assigned. This means that the same value is just repeated to all the splitted objects. This took take place at the beggining before assigning this.
  • It seems that if there are more than one aggregation types, and they have the same field name the process will fail since the column name is changed.

Santonia27 and others added 17 commits May 8, 2024 16:06
@Santonia27
Merge branch 'main' of https://github.com/Deltares/hydromt_fiat into …
95bc1a0 
…#318_new_composite_area_aggregation
@Santonia27
remove file
4bd4e91
@Santonia27
add file
5df0405
@Santonia27
Merge branch 'main' of https://github.com/Deltares/hydromt_fiat into …
f75d048 
…#318_new_composite_area_aggregation
@Santonia27
fix split composite area code
8413be5
@Santonia27
improve new_composite_area code
a3f8cc8
@Santonia27
remove unneccessary code lines
7306890
@Santonia27
updated files
c876503
@panosatha
fixed bug that renaming was in an else statement
809edb1
@panosatha
Merge branch '#318_new_composite_area_aggregation' of https://github.…
800aa1a 
…com/Deltares/hydromt_fiat into #318_new_composite_area_aggregation
@Santonia27
split max potential damages for new composite area
574ced3
@Santonia27
fix parameter list
9a08f2d
@Santonia27
update split_max_potential_damage function
c281fe0
@Santonia27
divide between cont and structure when splititing damages for new com…
57a19ca 
…posite area
@Santonia27
place empty list out of loop
2d86cc2
@Santonia27
merge new max structure and content to old exposuregdf
c8ec20c
@Santonia27
round max potential damages
1a3ec5e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panosatha panosatha panosatha left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Santonia27 @panosatha