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

[Admins] Add Area type and allow only Point on Locality #72

Merged
merged 5 commits into from
Nov 9, 2023

Conversation

DavidKarlas
Copy link
Collaborator

With new relations we can now add multiple different geometries to one locality, here we are changing modeling for localities to always have point and in case it also has polygon adds it via new Area type which references Locality via localityId property.

Also included few minor changes into this PR:

  • Removed mentioning disputed boundaries as missing feature. This was added few weeks ago, didn't realize it was mentioned here...
  • Changed context property/relation name to contextId to reflect new guidance

With new relations we can now add multiple different geometries to one admin, here we are changing modeling for localities to always have point and in case it also has polygon adds it via new Area type which references Locality via `localityId` property.
@jenningsanderson
Copy link
Collaborator

View the changes to documentation and schema here: https://dfhx9f55j8eg5.cloudfront.net/pr/72

Copy link
Collaborator

@jenningsanderson jenningsanderson 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 — would calling it administrativeArea instead of just area be more descriptive?

area is a bit of an overloaded term in geospatial, so I think it could be helpful to more clearly delineate that.

Copy link
Contributor

@ibnt1 ibnt1 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, left couple minor comments, thanks!

schema/admins/area.yaml Outdated Show resolved Hide resolved
schema/admins/area.yaml Outdated Show resolved Hide resolved
Also renamed `maritime` to `isMaritime` and moved to `defs.yaml` in `admins/`
Also added to `locality.mdx` text mentioning where to get polygons, to help new comer faster understand this pretty important information when looking at admins :)
@jwass
Copy link
Collaborator

jwass commented Nov 3, 2023

Looks good to me — would calling it administrativeArea instead of just area be more descriptive?

area is a bit of an overloaded term in geospatial, so I think it could be helpful to more clearly delineate that.

I kind of agree but also it's within the admins theme so I feel like adding administrative is redundant. As I was reviewing I was thinking we might just want locality, boundary, and area or something like that. I agree that area is a bit of an odd duck here so maybe there's a better word (polygon?).

@jenningsanderson
Copy link
Collaborator

jenningsanderson commented Nov 4, 2023

Looks good to me — would calling it administrativeArea instead of just area be more descriptive?
area is a bit of an overloaded term in geospatial, so I think it could be helpful to more clearly delineate that.

I kind of agree but also it's within the admins theme so I feel like adding administrative is redundant. As I was reviewing I was thinking we might just want locality, boundary, and area or something like that. I agree that area is a bit of an odd duck here so maybe there's a better word (polygon?).

Yeah — the only problem is that type is currently unique globally, not per-theme, so area or polygon would be forever associated only with this particular theme.

At least, that's how it's currently in the schema, but maybe it doesn't matter if type is reused b/n themes?

@jwass
Copy link
Collaborator

jwass commented Nov 5, 2023

Yeah — the only problem is that type is currently unique globally, not per-theme, so area or polygon would be forever associated only with this particular theme.

At least, that's how it's currently in the schema, but maybe it doesn't matter if type is reused b/n themes?

Interesting. I actually didn't think type was unique globally?? I think the schema is usually seeing if both a theme and type match, then dispatching to the right type. Still have to check but if it is the case, would you think area is okay? Or still prefer administrativeArea?

@DavidKarlas
Copy link
Collaborator Author

I just want to add I"m super happy with current name of localityArea because it makes it super clear it adds areas to type before it which is locality...

@jwass
Copy link
Collaborator

jwass commented Nov 5, 2023

I just want to add I"m super happy with current name of localityArea because it makes it super clear it adds areas to type before it which is locality...

Sounds good. I guess for consistency, should administrativeBoundary then be localityBoundary or something like that? Seems weird to have administrative only there but nowhere else.

Tristan raised concern regarding what would landmass version of polygon represent, simplified area or not(because not simplified is very big and complex geometry) and all this can be calculated by user by removing Coastline from maritime Polygons...
Hence let's have broader discussion over next few weeks which polygons we want to include in Overture...
@DavidKarlas
Copy link
Collaborator Author

FYI, I removed isMaritime property from localityArea because it needs more discussion... Since Landmass area type can be derived from data + I would like to use subType instead of property...

schema/admins/locality.yaml Show resolved Hide resolved
schema/admins/localityArea.yaml Outdated Show resolved Hide resolved
@DavidKarlas
Copy link
Collaborator Author

Got all needed approvals and resolved all open comments

@DavidKarlas DavidKarlas merged commit 97155a6 into dev Nov 9, 2023
2 checks passed
@jenningsanderson jenningsanderson deleted the users/davidkarlas/addAreaType branch November 9, 2023 15:36
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.

6 participants