Closed nasaownsky closed 3 years ago
@colincadams Check the latest changes and approve if they're right.
I don't think there is any fixture data that works with the 10% threshold unfortunately, but the last point for CO only covers 25% and the rest cover more so I upped the threshold to 30% to test.
before:
after:
It looks to me like instead of dropping the last incarceration rate point as expected, it instead just shifted the line one month to the left. I also think we will want this to affect both key insights and the charts (@hobuobi ?)
@jessex as @colincadams noted testing can only be performed with manual changing either threshold level or percentage_covered_population
in jails fixture file.
@colincadams I've fixed that bug!
@colincadams would you mind taking over review for this one since you've already got a handle on it? Thanks!
@colincadams About implementation. If, for example, latest displaying rate has constraint, all key insights should display data month back? Or just rate?
@nasaownsky that is a great thought! Discussed with @hobuobi and we agree it makes sense to limit all key insights based on the percentage covered for incarceration rate. So if the constraint applies for a particular month, all statewide data for that month should be dropped.
@colincadams Could you include percentage_covered_population
field in PERCENTAGE_COVERED_COUNTY
metric in jails fixture file?
I can add that, and then you could limit both INCARCERATION_RATE_JAIL
and PERCENTAGE_COVERED_COUNTY
based on their own percentage_covered_population
field, but you would still have to limit POPULATION_JAIL
based on the percentage_covered_population
of one of the other two metrics since it will have it's own value that may be different. In that case is it still useful to add the field to PERDCENTAGE_COVERED_COUNTY
?
@colincadams sorry for bother you, I managed to add constraint without any new data. Check this out.
Not a bother at all, thanks for helping us think through the edge cases here! This seems to be working well now, and I think the approach is reasonable. Thanks for fixing the problem when percentage covered was null also!
Posting screenshots to capture the behavior change:
Description of the change
Type of change
Related issues
Checklists
Development
These boxes should be checked by the submitter prior to merging:
Code review
These boxes should be checked by reviewers prior to merging: