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

Add support of requires_tables multipolygons #1818 #1859

Merged
merged 6 commits into from
May 18, 2023
Merged

Add support of requires_tables multipolygons #1818 #1859

merged 6 commits into from
May 18, 2023

Conversation

frodrigo
Copy link
Member

@frodrigo frodrigo commented May 8, 2023 •
edited
Loading

Implement a new common required table for multipolygons. See #1818.

Use it in a first analyser as demo analyser_osmosis_polygon_small.

TODO:

  • Update the doc
  • Add warning, contains only closed and borders inside polygon
  • Use LEFT JOIN to fail on missing members
  • Remove version, user_id, tstamp, changeset_id

@frodrigo frodrigo requested a review from Famlam May 8, 2023 19:25
@Famlam
Copy link
Collaborator

Famlam commented May 9, 2023 •
edited
Loading

Looks great!
I build some difficult test cases in order to test the table (PR @ frodrigo#9, probably good to have; not specifically because this analyser is complicated but since it would show errors if the table is incorrectly modified later) and they all pass.

(Also to keep in mind for later: the docs probably need to contain this table where buildings, highways etc are also listed)

@frodrigo
Copy link
Member Author

@Famlam lot of good feedback, thank you. I have no free time to work on this in the upcoming week. If you wish, you can work on this on a new branch under osm-fr/osmose-backend, so latter we could both work on it.

@Famlam Famlam mentioned this pull request May 13, 2023
Closed
@Famlam
Copy link
Collaborator

Famlam commented May 13, 2023

@Famlam lot of good feedback, thank you. I have no free time to work on this in the upcoming week. If you wish, you can work on this on a new branch under osm-fr/osmose-backend, so latter we could both work on it.

Welcome :)
I've implemented it in #1861
I'm just not sure what you mean with Add warning, contains only closed and borders inside polygon

@frodrigo
Copy link
Member Author

I'm just not sure what you mean with Add warning, contains only closed and borders inside polygon

I add it.

I change the having close to a simplest one.

Famlam
Famlam approved these changes May 18, 2023
View reviewed changes
Copy link
Collaborator

@Famlam Famlam left a comment

Choose a reason for hiding this comment

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

Looks good to me :)
(I haven't tested the diff mode though, but I assume it'll include changes in inner and outer members as it's basically the same as a regular relation diff)

@frodrigo
Copy link
Member Author

Let's merge it!

frodrigo and others added 5 commits May 18, 2023 20:10
@frodrigo
Add support of requires_tables multipolygons osm-fr#1818
f7d80e8
@Famlam @frodrigo
Add tests
2a1e2e5 
Primarily to cover the proper functioning of table multipolygons. Includes:
- complex multipolygon (below the treshold, i.e. should match)
- multipolygon with outer > treshold, but outer+inner < treshold (i.e. should match)
- multipolygon with mini outer + big outer (should not match)
- multipolygon simulated not fully in extract (should not match)
@Famlam @frodrigo
Drop incomplete MP
9f699c0
@Famlam @frodrigo
Update docs
6bbf737
@frodrigo
Also check multipolygon on analyser_osmosis_polygon_small osm-fr#1818
1a2d189
@frodrigo frodrigo merged commit 900c73b into osm-fr:dev May 18, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Famlam Famlam Famlam approved these changes

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
@frodrigo @Famlam