-
Notifications
You must be signed in to change notification settings - Fork 11
Add admin_level to divisions #419
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
base: dev
Are you sure you want to change the base?
Conversation
schema/divisions/division.yaml
Outdated
| admin_level: | ||
| description: | ||
| Integer representing this division's position in its country's administrative | ||
| hierarchy. Lower numbers correspond to higher-level administrative units. | ||
| type: integer | ||
| minimum: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not really consistent in the schema (especially within the divisions theme IIRC) about whether we want to start numbered sequences at 0 or 1.
My mental model is 0 is the programmer way, 1 is the regular human way. For some reason I tend toward thinking 1 is better as the starting point here, but I don't have a strong justification for it.
Why zero, or why one?
This value being repeated in 3 places, it probably makes sense to define it reusably in schema/divisions/defs.yaml and reuse it.
The parallel structure in the Pydantic code base would be a NewType within the divisions package named AdminLevel.
Can we be a bit more precise in the documentation?
Here's a stab at it:
Integer representing this division's position in its country's administrative hierarchy, where
lower numbers correspond to higher level administrative units.
In Overture data releases, this value is equal to the number of ancestor features in the
division's primary hierarchy (the one defined by following `parent_division_id`). Thus, a
country always has an `admin_level` of 0, a region has an `admin_level` of 1, while further
subdivisions have `admin_level` values greater than 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might have lost some context from the previous pr. the accepted standard admin 0/1/2/3. ideally we dont want to create or own standard that doesnt align. as noted, osm has their own which does not follow the majority of providers of boundary data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why zero, or why one?
Yeah, I think this is answered in Jonah's comment, but we're really just borrowing from the industry standard admin0/admin1/admin2 convention here.
This value being repeated in 3 places, it probably makes sense to define it reusably in schema/divisions/defs.yaml and reuse it.
A more recent commit does just this - replaces these three values with a new entry in the defs.yaml file.
Can we be a bit more precise in the documentation?
Thanks for this! I've updated the definition of this new property using your suggestion, with a minor tweaks.
| present on boundaries between different principal subdivisions or countries. | ||
| allOf: | ||
| - "$ref": "../defs.yaml#/$defs/propertyDefinitions/iso3166_2SubdivisionCode" | ||
| admin_level: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to think about this for a second given that it occurs on the boundary feature type, but I recognize it is right: boundaries are only meaningful at "shared" levels of the administrative hierarchy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's correct.
vcschapp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aligned, this seems right!
I'm clicking request changes because ... shouldn't admin_level be a required field on all three feature types? If it is fully derived from the level in the feature hierarchy, which I think it is, then there's no reason why every feature shouldn't have it is there?
|
@stepps00 , we just merged the |
So I'm hesitant to mark this as required for all fields. Two main reasons:
Edit: See comment below; I've set this as required for relevant subtypes.
This should be complete in the two most recent commits in this branch.. I'll take a look at the validation failures. |
Update: in the most recent set of commits, I've set this new field as a required property on subtypes above localadmin in the subtype hierarchy (example). Validation should now pass with a updated Pydantic baseline schema files. |
Will replace #414.
A. Expected release date
Dec 2025
Description
This change:
admin_levelfield (int value) representing the division’s position in its country’s hierarchyChecklist
Checklist of tasks commonly-associated with schema pull requests. Please review the relevant checklists and ensure you do all the tasks that are required for the change you made.
Abut is not intended to test propertyA's validity, and you made a schema change that invalidates propertyAin that counterexample, fix the counterexample to align it with your schema change.Documentation website
Update the hyperlink below to put the pull request number in.
Docs preview for this PR.