USAFacts Prop Fix

[dshemetov]

Description

Same half-correct prop signal value that JHU was having #328

Changelog

• Filtered out megafips when aggregating up to state, same fix as above

@sgsmob can you foresee any problems with this sort of fix here? Passed my sanity checks, but I'm not familiar with this indicator.

Fixes

Fixes #536

[dshemetov]

Seems to work when comparing local (receiving files) and remote (API) (this is CA for example)

Edgecases?

• megafips counties should be fine, if USAfacts uses it, as the population zero only happens at the state aggregation level
• msa and hrr are fine, they don't use the geomapper

[dshemetov]

Also a longterm thought: the way fips to state conversion works GeoMapper seems to be dangerous for prop signals. Maybe it would be best to update the function to filter out megafips by default. I don't think there are cases where we want to keep the megafips data when converting to state?

[capnrefsmmat]

Let me suggest that we also need a unit test here. This bug has already popped up twice in different indicators; if there's a way to test geo_map by passing in a fake data frame with a few different counties and ensuring the props are correct, we should definitely do it.

[krivard]

I don't think there are cases where we want to keep the megafips data when converting to state?

Can you be more specific? Because I'm pretty sure we only want to jettison megafips population -- numerator counts etc should be retained.

[krivard]

Let me suggest that we also need a unit test here.

Agree this PR should include a unit test to prevent regression in future. @dshemetov? nertz, pacific time. My morning is free so I'll pick up the test.

This bug has already popped up twice in different indicators

...unfortunately I don't think it's possible to test the current behavior in a way that's robust to adding new indicators susceptible to the same problem. We'd have to shift prop computations into the utility to do that -- which might be a good idea regardless -- but is too big a task for a hotfix like this. I'll put it on the schedule for December at least, and see if there's anything we can rearrange to get a head start on it this month.

[krivard]

Test is in; verified fails in main and succeeds in this PR.

[sgsmob]

This bug has already popped up twice in different indicators

...unfortunately I don't think it's possible to test the current behavior in a way that's robust to adding new indicators susceptible to the same problem. We'd have to shift prop computations into the utility to do that -- which might be a good idea regardless -- but is too big a task for a hotfix like this. I'll put it on the schedule for December at least, and see if there's anything we can rearrange to get a head start on it this month.

I don't understand why you can't test this.

[sgsmob]

Overall this looks pretty good to me. I don't have a ton of USAFacts background to know if this is going to cause more issues, but if it worked in JHU, that seems pretty good.

[sgsmob]

Are these always assumed to be 0?

[krivard]

No, but it was the minimum change to the tests that failed in main and succeeded with the fix.