Skip to content

Finish quidel geo util refactor #665

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

Closed
wants to merge 5 commits into from
Closed

Finish quidel geo util refactor #665

wants to merge 5 commits into from

Conversation

chinandrew
Copy link
Contributor

Description

For some reason quidel_covidtest was refactored but quidel wasn't finished. Basically just copied the changes from b39d55f

Changelog

Itemize code/test/documentation changes and files added/removed.

  • Update geo_map to use geo utils by taking existing code from quidel_covidtest
  • Change function calls to match new signature
  • Fix tests

Fixes

  • Fixes #(issue)

@jingjtang
Copy link
Contributor

jingjtang commented Jan 5, 2021

@chinandrew I got these for linting. No big problems.

************* Module delphi_quidel.run
delphi_quidel/run.py:7:0: W0611: Unused join imported from os.path (unused-import)
delphi_quidel/run.py:9:0: W0611: Unused pandas imported as pd (unused-import)
************* Module delphi_quidel.pull
delphi_quidel/pull.py:65:0: C0301: Line too long (102/100) (line-too-long)
delphi_quidel/pull.py:314:0: C0301: Line too long (115/100) (line-too-long)
delphi_quidel/pull.py:405:0: C0301: Line too long (117/100) (line-too-long)
************* Module delphi_quidel.data_tools
delphi_quidel/data_tools.py:9:0: C0303: Trailing whitespace (trailing-whitespace)
delphi_quidel/data_tools.py:67:0: C0301: Line too long (109/100) (line-too-long)
delphi_quidel/data_tools.py:122:0: C0301: Line too long (112/100) (line-too-long)
delphi_quidel/data_tools.py:175:0: C0301: Line too long (105/100) (line-too-long)
delphi_quidel/data_tools.py:310:0: C0301: Line too long (102/100) (line-too-long)

Other than these, the logic looks fine. All tests passed

@chinandrew
Copy link
Contributor Author

@krivard informed me that the run-quidel branch already has these changes and just isn't in main yet, so I guess this may not be needed?

@jingjtang
Copy link
Contributor

jingjtang commented Jan 5, 2021

@chinandrew Oh, yes. I have finished the geo util refactor in run-quidel branch. Didn't realize that it is not in the main. Sorry about the duplicate work that you did. The work still left for Quidel is:

  1. Add megacounties
  2. Add HHS and national level data

@krivard
Copy link
Contributor

krivard commented Jan 5, 2021

run-quidel has the merged flu+covid quidel pipeline, which has not been deployed, and iirc is blocked on deletion support since it will also remove a bunch of regions from the historical data record

@chinandrew
Copy link
Contributor Author

is run-quidel what's currently deployed?

@jingjtang
Copy link
Contributor

@chinandrew quidel_covidtest is the one that is deployed which is for covidtest only. run-quidel is for both covid test and flu test

@chinandrew
Copy link
Contributor Author

ah, right. in that case I think this PR can be closed and we'll just figure out the run-quidel issue

@chinandrew chinandrew closed this Jan 5, 2021
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.

3 participants