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

ocds for coming "2019 United Kingdom general election" and existing parliament #181

Merged
merged 16 commits into from
Nov 5, 2019
Merged

ocds for coming "2019 United Kingdom general election" and existing parliament #181

merged 16 commits into from
Nov 5, 2019

Conversation

sguenther85
Copy link
Contributor

No description provided.

identifiers/country-gb/countries.csv Outdated Show resolved Hide resolved
identifiers/country-gb/countries.csv Outdated Show resolved Hide resolved
identifiers/country-gb/regions-eng.csv Outdated Show resolved Hide resolved
identifiers/country-gb/constituencies.csv Outdated Show resolved Hide resolved
@jloutsenhizer
Copy link
Contributor

I was taking another look and realized I overlooked the OCD ID repository already contains United Kingdom OCD IDs using "uk" instead of "gb" for the country code from #156

I'm inclined to prefer the gb country code since it's the correct ISO 3166-1 alpha-2 code for United Kingdom. Perhaps we can alias the uk OCD IDs to the new gb ones?

@jpmckinney @sguenther85 what do you think?

@jpmckinney
Copy link
Member

@jloutsenhizer Yes, I missed that before merging #156. We should create the aliases, as the committers for #156 are using those codes.

@sguenther85
Copy link
Contributor Author

@jloutsenhizer @jpmckinney
I can do that, but only "country-uk.csv", and "uk_parliament_consitituencies.csv" will match to the constituencies from here.
I didn't find the regions like "ocd-division/country:gb/country:gb-eng/region:ukg,West Midlands" in the uk files. Also aliases for the "parts" are missing.

So we would have only the aliases for "gb" and all constituencies.
Should I do it anyway, even if some aliases were missing?

@jloutsenhizer
Copy link
Contributor

Yes, I think adding aliases only for the ones which are currently duplicated in this PR is fine. I think having duplicate OCD IDs for the same divisions is worse than having some inconsistency in the canonical OCD IDs.

@sguenther85
Copy link
Contributor Author

Ok. @jloutsenhizer @jpmckinney
Could you take anouther look?
Have I forgotten anything?

Copy link
Contributor

@jloutsenhizer jloutsenhizer left a comment

Choose a reason for hiding this comment

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

Aliases overall look good to me. I'm not sure whether we need to remove the original "uk" ocd IDs, @jpmckinney would know better.

Only thing left from me is missing headers on some of the csv files.

identifiers/country-gb/regions-eng.csv Show resolved Hide resolved
identifiers/country-gb/constituencies.csv Show resolved Hide resolved
@jpmckinney
Copy link
Member

For the old OCD-IDs, @jloutsenhizer can you create a new issue tagging the participants from #156 about how we should transition to the gb OCDIDs? I figure we can either remove the old IDs (creating aliases where possible), or add a 'deprecated' column, if a hard transition isn't an option.

@jpmckinney
Copy link
Member

I re-read the OCDEP – I think perhaps we should be using "corrections" files instead of aliases. Can we remove the aliases from this PR, and move that discussion to the proposed new issue?

I also notice that the uk IDs don't have unnecessary nesting, i.e. all UK Parliament constituencies are right under country:uk, instead of nested under part and region. This is following the style in the OCDEP, e.g.:

Cary, North Carolina (note that despite being within Wake County this is not indicated due to not being an identifying feature)
ocd-division/country:us/state:nc/place:cary

New York City, City Council District 36 (happens to be in Brooklyn- but not significant to include in id)
ocd-division/country:us/state:ny/place:new_york/council_district:36

Can we remove any unnecessary hierarchy in this PR? We'd only need it if if was necessary to disambiguate two otherwise identical IDs.

@jloutsenhizer
Copy link
Contributor

Created #184 to continue discussion on migrating the country:uk OCD IDs.

As for these identifiers,
I find the OCDEP 2 documentation a bit vague. In particular, it's not defined what it means for something to be an "identifying feature", which is only referenced in the example identifiers. As far as I can tell this has been used for disambiguation.

However, this leads to some strange IDs. For example, in Westmoreland County in Pennsylvania, there is a Donegal Township and a borough named Donegal. The OCD IDs respectively are:
ocd-division/country:us/state:pa/county:westmoreland/place:donegal
ocd-division/country:us/state:pa/place:donegal
It's not very clear why one has the county included as part of the OCD ID and one does not.

There's also places in Pennsylvania like Murrysville, which doesn't collide with the name of any other place in the United States, but we still include the state as a part of the OCD ID:
ocd-division/country:us/state:pa/place:murrysville

In my opinion, it makes more sense to include the full path in the OCD ID when it's part of how the place is defined. In the case of these parliament constituencies, they are defined as subdivisions of the different parts of the UK, and within England as different subdivisions of larger regions within England.

@jpmckinney
Copy link
Member

Sounds fine to me, if that is indeed how they are defined (e.g. by a boundary commission). Feel free to merge once the aliases are removed from this PR.

@sguenther85
Copy link
Contributor Author

Aliases have been removed again ;)

@jpmckinney
Copy link
Member

Thank you for your patience :)

@jpmckinney jpmckinney merged commit 45f7a6f into opencivicdata:master Nov 5, 2019
chris48s added a commit to chris48s/ocd-division-ids that referenced this pull request Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants