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

Drop computable Rankings.percentage and provide func #193

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chadmiller
Copy link
Contributor

At model instance .save() time, we were calculating the value of one column
using two others.

Since the type of that column is an IntegerField but elsewhere on the site we
round to one digit after decimal, a reader complained about the inconsistency
(really that it was a floor() operation).

Instead, this is the first part of the whole fix, but someone else can
"makemigrations" to discard the db column at leisure.

Closes #184

At model instance .save() time, we were calculating the value of one column
using two others.

Since the type of that column is an IntegerField but elsewhere on the site we
round to one digit after decimal, a reader complained about the inconsistency
(really that it was a floor() operation).

Instead, this is the first part of the whole fix, but someone else can
"makemigrations" to discard the db column at leisure.

Closes ebmdatalab#184
@chadmiller
Copy link
Contributor Author

I didn't actually test this. Sorry.

@sebbacon
Copy link
Contributor

Thanks for the PR!

However, percentage is deliberately a field so we can sort and filter by it (e.g. here).

Now, I'm not sure anyone cares about sorting or filtering by percentage in the API, so perhaps this is an opportunity to excise unused code - in our frontend we sort in javascript, and anyone else can sort how they want once they've downloaded.

However, if we did that, we should change how rankings are computed as @NickCEBM has said previously he's going to find historic rankings useful for future analyses - right Nick?

All in all, I think there's an argument here (given we don't actually know if/how others are using the API, and the extra work) for continuing to set the value as a database field, but matching how we round it elsewhere.

@NickCEBM
Copy link
Contributor

Yes, historic rankings are a very nice thing to have for a number of reasons.

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.

Rounding in sort column
3 participants