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

Local authority eng codes #275

Merged
merged 2 commits into from
Dec 20, 2016

Conversation

symroe
Copy link
Contributor

@symroe symroe commented Dec 5, 2016

This PR adds codes from the local-authority-eng register.

I checked the map file in to the data directory as the remote location of that file isn't stable, even though the content is.

The thing I'm least sure about is 06cc1ff. Guessing at the CodeType from a regex seems to work for the other IDs, but a three letter code isn't unique to local-authority-eng (for example, we have local-authority-wls coming, with the same ID system).

One option would be to convert the query to an __in query, with each regex block appending to the list of CodeTypes. This feels like a fairly large change with possible performance implications, so I thought it better to add it as it's own commit that I can remove if it's too tricky to get done in this PR.

@dracos dracos added the Current label Dec 5, 2016
@dracos
Copy link
Member

dracos commented Dec 5, 2016

Thanks! There is a better lookup to use if you have a code, e.g. /code/local-authority-eng/AMB – the fact /area/ lets you use ONS/GSS codes on top of ID is historical, and we probably don't want to add any new things to that.

@symroe
Copy link
Contributor Author

symroe commented Dec 5, 2016

Oh cool! I use /area/[gss] all the time, never knew about /code/[CodeType]/[Code].

I'll remove 06cc1ff and force push this branch.

@@ -386,7 +386,7 @@ class CodeType(models.Model):
# This could be extended to a more generic data store of information on an
# object, perhaps.

code = models.CharField(max_length=10, unique=True, help_text="A unique code, eg 'ons' or 'unit_id'")
code = models.CharField(max_length=100, unique=True, help_text="A unique code, eg 'ons' or 'unit_id'")
Copy link
Member

Choose a reason for hiding this comment

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

As type.code and code.code both went up to 500 when they were migrated, shall we just do the same here for consistency? And we might as well do nametype.code up to 500 as well, then they all are :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed 4357e30 that makes these changes.

E10000023 local-authority-eng:NYK North Yorkshire
E07000154 local-authority-eng:NOR Northampton
E10000021 local-authority-eng:NTH Northamptonshire
E06000057 local-authority-eng:NBL Northumberland
Copy link
Member

Choose a reason for hiding this comment

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

Should this file also have other GSS codes that match to the same local authority? e.g. here, E06000048 is also Northumberland.

Copy link
Member

Choose a reason for hiding this comment

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

There are 8 councils with multiple GSS codes - Glasgow, East Dunbartonshire, St Albans, Welwyn Hatfield, East Hertfordshire, Stevenage, Northumberland, Gateshead.

Copy link
Member

Choose a reason for hiding this comment

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

This of course then raises another issue as two areas in MapIt would have the same code, and /code/ currently returns an error when that happens. No reason it couldn't return a list if multiple results, though...

Copy link
Member

Choose a reason for hiding this comment

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

#127 would presuambly 'solve' that issue, if it allowed the concept to have its own IDs (of which this would certainly be one), separate from its boundaries (which have the GSS IDs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't reproduce this. I've added a new line to the map file for E06000048/Northumberland/NBL and /code/local-authority-eng/NBL still works.

Of course, I should add the extra GSS->ID maps in the file in the local-authority-eng repo. Do you have a full list of them, or should I read through the mapit_UK_fix* scripts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also Code is unique on ('area', 'type'), so I'm not sure how this would ever work without removing that constraint.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, yes, so the example I gave was the same code in areas of two different types (OLF and OLG). Probably easiest for this for now if we just put them on the "latest" GSS codes (ie. what you have already done) and then raise a new ticket to do something about it? This will have a deadline of as soon as ONS issue a new GSS code for a local authority...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's quite soon! But I agree, this should be a different issue. Are you happy to write it, as you will have a better idea of how to express it. I'm happy to have a go at working on it if you like.

Copy link
Member

Choose a reason for hiding this comment

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

Is it? They don't do it very often, or we'd probably have had to do something properly sooner! :) I've added a comment on #127 whereby we could add parent areas to the councils that remain stable through such boundary changes, then these codes could theoretically be placed on there instead...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry, I'm thinking about wards, where there are 13 counties getting new ward codes next May. That doesn't change the council geography GSS codes though, so we're fine.

This is because the name of the 'local-authority-eng' is longer
than the previous 10 characters allowed.

# Add Codes
data_path = os.path.abspath(
os.path.join('mapit_gb', 'data', 'gss-to-local-authority-eng.tsv'))
Copy link
Member

Choose a reason for hiding this comment

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

This means it only works if you run manage.py from inside its own directory? Could it use __file__ in some way so that it doesn't matter - there are some other uses in this repo.

area=gss_code.area
)
except Code.DoesNotExist:
pass
Copy link
Member

Choose a reason for hiding this comment

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

Presumably we want to know if we're missing a GSS code? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you want to know? Is there some logging system, or is printing to stdout ok?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, printing out is find, probably to self.stderr, I imagine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've made that change :)

@symroe symroe force-pushed the local-authority-eng-codes branch 2 times, most recently from ee5c713 to 266ec0b Compare December 6, 2016 08:23
@dracos dracos merged commit 9f6f2eb into mysociety:master Dec 20, 2016
@dracos dracos removed the Current label Dec 20, 2016
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