feat: add min_issue to covidcast meta data #236

sgratzl · 2020-10-07T11:05:45Z

closes #232

removes some unneeded whitespace

closes #232

krivard · 2020-10-08T20:46:14Z

Fails integration tests:

$ docker run --rm --network delphi-net delphi_python python3 -m undefx.py3tester.py3tester repos/delphi/delphi-epidata/integrations
[...]
======================================================================
FAIL: test_round_trip (repos.delphi.delphi-epidata.integrations.server.test_covidcast_meta.CovidcastMetaTests)
Make a simple round-trip with some sample data.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/src/app/repos/delphi/delphi-epidata/integrations/server/test_covidcast_meta.py", line 87, in test_round_trip
    self.assertEqual(response, {
AssertionError: {'res[250 chars]ue': 1, 'max_issue': 2, 'min_lag': 0, 'max_lag[4262 chars]ess'} != {'res[250 chars]ue': 2, 'max_issue': 2, 'min_lag': 0, 'max_lag[4262 chars]ess'}
Diff is 22152 characters long. Set self.maxDiff to None to see it.

======================================================================
FAIL: test_suppress_work_in_progress (repos.delphi.delphi-epidata.integrations.server.test_covidcast_meta.CovidcastMetaTests)
Don't surface signals that are a work-in-progress.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/src/app/repos/delphi/delphi-epidata/integrations/server/test_covidcast_meta.py", line 257, in test_suppress_work_in_progress
    self.assertEqual(response, {
AssertionError: {'res[250 chars]ue': 1, 'max_issue': 2, 'min_lag': 0, 'max_lag[4262 chars]ess'} != {'res[250 chars]ue': 2, 'max_issue': 2, 'min_lag': 0, 'max_lag[4262 chars]ess'}
Diff is 22152 characters long. Set self.maxDiff to None to see it.

melange396

i believe this is not going to work as intended, and instead will show the issue date of the least-recently-updated of the issues in a group, which i would argue is meaningless (plz correct me if im wrong). i think it will require a MIN(issue) inside the sub-SELECT and appropriate handling outside of that.

also, some non-trivial test cases would be nice, with different issue values.

melange396 · 2020-10-08T21:44:35Z

src/acquisition/covidcast/database.py

@@ -576,6 +576,7 @@ def get_covidcast_meta(self):
        ROUND(AVG(`value`),7) AS `mean_value`,
        ROUND(STD(`value`),7) AS `stdev_value`,
        MAX(`value_updated_timestamp`) AS `last_update`,
+        MIN(`issue`) as `min_issue`,


i believe this is not going to work as intended, and instead will show the issue date of the least-recently-updated of the issues in a group, which i would argue is meaningless (plz correct me if im wrong). i think it will require a MIN(issue) inside the sub-SELECT and appropriate handling outside of that.

also, some non-trivial test cases would be nice, with different issue values.

true, I will think about it how to fix it. Moreover, we can think of using the is_latest_issue to find the max issue

@melange396 can you take a look at the new statements. I split it into two, since I couldn't think of a fast way to handle both cases at the same time. Moreover, I switched the nested statement to the is_latest_issue flag. It is not that easy to test for me, since I don't have access to the real database but just local test data

i suppose this will work, but the old SQL query would have done the trick if you added min(issue) min_issue`` to the nested select... i think that would be preferable to adding a bunch of extra lines unless you can think of another benefit that justifies the extra complexity

yes but I think this version should be more efficient using is_latest_issue instead of a join

after wrestling with and waiting for long-duration queries for the past week, i think it might be wise to test the performance of the one-query vs two-query approaches on the staging server. i can certainly help do this if necessary.

Apologies for being so late to this conversation.

The use case for min_issue is briefly described in the source issue for this PR, but the jist is this: A researcher is constructing as_of queries and wants to know the earliest date they can pass in and still expect a non-nil result. This means that min_issue must not be restricted to rows with is_latest_issue=1, since that would hide issues which are not the most recent but nevertheless accessible using as_of.

i will go ahead and try this and other variants out on the staging copy of the database to check performance and runtimes

melange396 · 2020-10-14T23:34:55Z

src/acquisition/covidcast/database.py

@@ -576,6 +576,7 @@ def get_covidcast_meta(self):
        ROUND(AVG(`value`),7) AS `mean_value`,
        ROUND(STD(`value`),7) AS `stdev_value`,
        MAX(`value_updated_timestamp`) AS `last_update`,
+        MIN(`issue`) as `min_issue`,


i suppose this will work, but the old SQL query would have done the trick if you added min(issue) min_issue`` to the nested select... i think that would be preferable to adding a bunch of extra lines unless you can think of another benefit that justifies the extra complexity

src/acquisition/covidcast/database.py

krivard · 2020-11-03T20:16:29Z

Putting this back on your radar @melange396 now that the JHU fires are out

sgratzl · 2021-05-05T13:47:14Z

closing this PR since meta generation has changed since then

feat: add min_issue to covidcast meta data

f723beb

closes #232

krivard requested a review from melange396 October 7, 2020 16:52

capnrefsmmat mentioned this pull request Oct 8, 2020

Reorder and update metadata docstring, convert last_update field to datetime cmu-delphi/covidcast#118

Merged

1 task

melange396 requested changes Oct 8, 2020

View reviewed changes

Merge remote-tracking branch 'origin/main' into sgratzl/min_issue

f177818

sgratzl marked this pull request as draft October 12, 2020 12:28

sgratzl added 2 commits October 12, 2020 16:26

fix: update min issue computation

6f01aa5

fix: python client test

b2fce76

melange396 reviewed Oct 14, 2020

View reviewed changes

sgratzl added 2 commits October 15, 2020 10:44

refactor: simpliy lookup

a581f0b

fix: test

06aab57

sgratzl marked this pull request as ready for review October 21, 2020 17:13

Merge remote-tracking branch 'origin/main' into sgratzl/min_issue

a85adbb

krivard requested a review from melange396 October 21, 2020 19:42

krivard mentioned this pull request Nov 11, 2020

Allow issues = "*" to get all issues of a single data point cmu-delphi/covidcast#260

Open

sgratzl closed this May 5, 2021

sgratzl deleted the sgratzl/min_issue branch May 5, 2021 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add min_issue to covidcast meta data #236

feat: add min_issue to covidcast meta data #236

sgratzl commented Oct 7, 2020

krivard commented Oct 8, 2020 •

edited

Loading

melange396 left a comment

melange396 Oct 8, 2020

sgratzl Oct 12, 2020

sgratzl Oct 12, 2020

melange396 Oct 14, 2020

sgratzl Oct 15, 2020

melange396 Oct 28, 2020

krivard Nov 6, 2020

melange396 Nov 6, 2020

melange396 Oct 14, 2020

krivard commented Nov 3, 2020

sgratzl commented May 5, 2021

feat: add min_issue to covidcast meta data #236

feat: add min_issue to covidcast meta data #236

Conversation

sgratzl commented Oct 7, 2020

krivard commented Oct 8, 2020 • edited Loading

melange396 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krivard commented Nov 3, 2020

sgratzl commented May 5, 2021

krivard commented Oct 8, 2020 •

edited

Loading