Skip to content

Add tall building areas, fixes to building areas and main#128

Open
einoyrjanainen wants to merge 1 commit into
mainfrom
feat--add-GeneralizeTallBuldingAreas
Open

Add tall building areas, fixes to building areas and main#128
einoyrjanainen wants to merge 1 commit into
mainfrom
feat--add-GeneralizeTallBuldingAreas

Conversation

@einoyrjanainen

Copy link
Copy Markdown
Collaborator

Description

Adds a new tall-building-areas algorithm that filters buildings by height class before delegating to the existing building areas workflow.

Also fixes GeneralizeBuildingAreas so it does not require building_function_id unless filtering is actually enabled, which avoids the KeyError when running the tall-building workflow with datasets that do not have that column.

Type of change

  • Bug fix

  • New feature

  • Other

  • Non-breaking change

  • Breaking change

Developer checklist

  • A CHANGELOG entry has been included (only when change is visible to users)

@JuhoErvasti JuhoErvasti left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'll leave a general comment; the code itself for the most part is OK, but IMO this does not need to be its own algorithm, since the only thing that's different is that some features are filtered out. I see these options:

a) we leave the filtering up to the downstream user(s)
b) we add the filter step to GeneralizeBuildingAreas
c) we add GeneralizeTallBuildingAreas but make it inherit GeneralizeBuildingAreas, which would let users control its parameters

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.

2 participants