Refactor safegraph to use geo utils

[chinandrew]

For #306.

Summary of changes:

• Convert safegraph indicator to use the GeoMapper utility.
• Remove old safegraph specific geo_maps file and code

[krivard]

Blocked on #325.

[dshemetov]

Tests all pass. One thing I noticed is that the process function in process.py is untested. It looks like it mostly just calls the other tested functions construct_signals and aggregate, so I don't feel too strongly on this, but just checking for code coverage sake.

Linter is mostly happy, except for in test_process.py, there are a few unused imports and trailing newlines.

All good after these get addressed.

[chinandrew]

Tests all pass. One thing I noticed is that the process function in process.py is untested. It looks like it mostly just calls the other tested functions construct_signals and aggregate, so I don't feel too strongly on this, but just checking for code coverage sake.

Linter is mostly happy, except for in test_process.py, there are a few unused imports and trailing newlines.

All good after these get addressed.

Assuming our deploy process is ok with it, would prefer those changes happen in a separate PR. That way this is just a refactor PR and there's a second "add tests" PR "fix linting" PR, each of which should preferably be small

[sgsmob]

FWIW some of these tests are being made more robust on the currently broken #309

[chinandrew]

FWIW some of these tests are being made more robust on the currently broken #309

Cool, are there any known gaps after this PR? If so, let's make an issue so it's tracked.

[krivard]

Assuming our deploy process is ok with it

On the policy/process side -- our deploy process more-or-less depends on merging each PR to production in turn. If one of the proposed tests identifies a bug that was introduced in this PR, it seems suboptimal to split them up. One alternative would be to create a staging branch for deploy-safegraph, merge the PRs in there, then once the full set of changes is complete and checked, PR to the deploy-safegraph branch proper. Thoughts welcome.

[chinandrew]

Assuming our deploy process is ok with it

On the policy/process side -- our deploy process more-or-less depends on merging each PR to production in turn. If one of the proposed tests identifies a bug that was introduced in this PR, it seems suboptimal to split them up. One alternative would be to create a staging branch for deploy-safegraph, merge the PRs in there, then once the full set of changes is complete and checked, PR to the deploy-safegraph branch proper. Thoughts welcome.

My idealistic response, which is well encapsulated by the google eng practices doc (CL = change list = PR) is that PRs should be as small as possible and change one thing. In this context, this PR just switches the previous fips to state code to the geoutil version, and is only concerned that that refactor works, which may or may not warrant writing additional tests -- depends on the functionality and current state of tests for the indicator and utils. In that sense I agree that if there are bugs introduced by this PR, they should be tested and be fixed in this PR. If there are bugs in unrelated code that depend on code changed in this PR but existed previously, i.e. the refactor is 100% fine but there's other issues, that's not in scope (see "Separate Out Refactorings") section of the above link.

Practically, if this doesn't agree with our deploy system and each merge needs to encapsulate as many possible changes and bug fixes as possible in one go, then we should do what we need to do to make it work (see "Don’t Break the Build" section).