Batch requests when requesting multiple days of data #275

JedGrabman · 2020-11-12T18:19:38Z

Description:
Improve efficiency of covidcast_days by requesting multiple days of data when it can be determined that it is safe to do so.

Change Log:

covidcast.R
- Update covidcast_days to query multiple days at once
- Create max_geo_values to find upper bounds on number of results returned
- Create warning messages for missing dates or missing geo_values
covidcast.R
- Add tests for added batching functionality
- Add basic documention for the use of mock and stub

sgsmob · 2020-11-12T20:49:00Z

R-packages/covidcast/R/covidcast.R

+  max_locations <- meta_info[meta_info$data_source == data_source &
+                              meta_info$signal == signal &
+                              meta_info$geo_type == geo_type, ]$num_locations
+  if (length(max_locations) == 0) {


Can you add a comment about

why the max locations would be empty and

why we default to number of geo values

?

I believe it's a pre-emptive fix for a similar effect of #270, but yeah, a comment would be good here

That's right, it's a defensive measure against signals that aren't documented in covidcast_meta and using the max as an upper bound when it can't be found. Added a comment.

R-packages/covidcast/R/covidcast.R

sgsmob · 2020-11-12T20:59:46Z

R-packages/covidcast/R/covidcast.R

+  } else {
+    nissues <- 1
+  }
+  max_days_at_time <- floor(3649 / (ngeos * nissues))


This number would be better refactored into a file-level constant who variable name is in all caps.

sgsmob · 2020-11-12T21:01:06Z

R-packages/covidcast/R/covidcast.R

+  if (max_days_at_time == 0) {
+    max_days_at_time <- 1
+  }
+  batch_days <- ceiling(ndays / max_days_at_time)


Is this actually just the number of batches? If so, would prefer to see this variable name changed to reflect that.

sgsmob · 2020-11-12T22:27:48Z

R-packages/covidcast/tests/testthat/test-covidcast.R

+      geo_value = "*",
+      as_of = NULL,
+      issues = NULL,
+      lag = NULL


should be indented more

sgsmob · 2020-11-12T22:28:24Z

Please add a description of this PR.

capnrefsmmat · 2020-11-12T23:02:20Z

This will also run into conflicts with #271, mainly just in the unit tests (should be easy to deal with). We should plan the order of operations here when we merge.

(but thanks -- this should be a great benefit to download speed)

sgsmob

Just the minor newline nit, but otherwise this is good to go.

sgsmob · 2020-11-13T19:36:10Z

R-packages/covidcast/tests/testthat/test-covidcast.R

+      ),
+      regexp = NA)
+  expect_called(m, 2)
+})


Add a newline here.

R-packages/covidcast/R/covidcast.R

Fix Python client shapefile packaging

capnrefsmmat

This looks good, thanks. Let's not merge yet -- I'd like to merge into r-pkg-devel instead of main, so let me adjust and get that working, and in the mean time you can fix a couple small nits

capnrefsmmat · 2020-11-19T00:21:18Z

R-packages/covidcast/R/covidcast.R

-               api_msg = dat[[i]]$message,
-               class = "covidcast_missing_geo_values"
-               )
+               msg = dat[[i]]$message,


This needs to go back to being called api_msg; maybe this changed back when you rebased on #252?

Suggested change

msg = dat[[i]]$message,

api_msg = dat[[i]]$message,

I had some merge conflicts early on, seems likely I grabbed the wrong line there. Fixed.

capnrefsmmat · 2020-11-19T00:24:38Z

R-packages/covidcast/DESCRIPTION

  grDevices,
  httr,
  jsonlite,
+  lubridate,


lubridate is only used in the tests, right? I think that means it can go in Suggests instead of Imports

ymd is a lubricate function which I used a few times in covidcast.R

Have python module test on installed package

Counts taken from line numbers of files in https://github.com/cmu-delphi/covidcast-indicators/tree/fb-package-validation/validator/static

Various small code fixes - Add comments - Fix indenting - Factor out constant (MAX_RESULTS)

- covidcast_days now takes max_geos as an optional parameter instead of determining it internally - max_geo_values is now specific_meta, for retrieving more than just num_locations.

capnrefsmmat · 2020-11-23T22:20:14Z

Well, that was satisfying to test out:

> foo = covidcast_signal("fb-survey", "smoothed_whh_cmnty_cli", start_day="2020-10-01", end_day="2020-11-20", geo_type="state")
Fetched day 2020-10-01 to 2020-11-20: 1, success, num_entries = 2601

That would have been 50 separate API calls before, so this is very nice.

JedGrabman requested review from capnrefsmmat and sgsmob as code owners November 12, 2020 18:19

JedGrabman linked an issue Nov 12, 2020 that may be closed by this pull request

covidcast::covidcast_signal - substantial speed improvement possible #231

Closed

sgsmob suggested changes Nov 12, 2020

View reviewed changes

JedGrabman requested a review from sgsmob November 13, 2020 19:22

sgsmob approved these changes Nov 13, 2020

View reviewed changes

capnrefsmmat mentioned this pull request Nov 13, 2020

Parallelize covidcast_signal()? #279

Closed

capnrefsmmat reviewed Nov 14, 2020

View reviewed changes

R-packages/covidcast/R/covidcast.R Show resolved Hide resolved

capnrefsmmat added the covidcast R package label Nov 14, 2020

capnrefsmmat mentioned this pull request Nov 14, 2020

Rollup of R package changes #281

Merged

JedGrabman force-pushed the dev-covidcast-efficiency branch from 0b0453d to e5d83a1 Compare November 16, 2020 22:09

capnrefsmmat mentioned this pull request Nov 18, 2020

Use CSV format for API access in R package #271

Merged

JedGrabman mentioned this pull request Nov 18, 2020

Covidcast CI R checks fail during "Set up R 3.5" step #302

Closed

JedGrabman force-pushed the dev-covidcast-efficiency branch from dd94b08 to b1e64d3 Compare November 18, 2020 19:26

fix shapefile

aa546a8

JedGrabman requested a review from capnrefsmmat November 18, 2020 21:27

capnrefsmmat and others added 5 commits November 18, 2020 16:45

Merge pull request cmu-delphi#306 from cmu-delphi/fixpackaing

397ebb4

Fix Python client shapefile packaging

Change to use editable install

6dee55b

update py version

02d6bf5

update develop.md

9d87813

Add noneditable CI mode

e376269

capnrefsmmat approved these changes Nov 19, 2020

View reviewed changes

capnrefsmmat and others added 6 commits November 19, 2020 11:23

Merge pull request cmu-delphi#309 from cmu-delphi/restructure-pytests

d40e68a

Have python module test on installed package

Batch multiple days in covidcast requests

5534ccf

Update constants based on historic observations

0cfbc8f

Counts taken from line numbers of files in https://github.com/cmu-delphi/covidcast-indicators/tree/fb-package-validation/validator/static

Handle date ranges in errors and dynamically determine max rows

eb5751d

Add tests for covidcast_days

1b893a5

handle queries for signals that are not in covidcast_meta

6ee3321

JedGrabman added 11 commits November 20, 2020 14:05

Linting, Clarity and Documentation

719e7b4

Function documentation

35f1937

Minor code cleanup

1fa0e4f

Various small code fixes - Add comments - Fix indenting - Factor out constant (MAX_RESULTS)

Add lubridate as explicit dependency

1a748d2

Make covidcast visible to testthat

8807044

Use stub instead of with_mock

9958eae

Update documentation

fffdc55

Refactor to avoid duplicate covidcast_meta calls

81066d1

- covidcast_days now takes max_geos as an optional parameter instead of determining it internally - max_geo_values is now specific_meta, for retrieving more than just num_locations.

Actually pass in max_geos

665f18a

minor lint changes

c53a247

msg -> api_msg

2eab488

JedGrabman force-pushed the dev-covidcast-efficiency branch from b1e64d3 to 2eab488 Compare November 20, 2020 19:05

capnrefsmmat changed the base branch from main to r-pkg-devel November 23, 2020 22:04

capnrefsmmat requested a review from chinandrew as a code owner November 23, 2020 22:04

capnrefsmmat merged commit 3de3b81 into cmu-delphi:r-pkg-devel Nov 23, 2020

capnrefsmmat mentioned this pull request Nov 23, 2020

Fix imports from dplyr and lubridate #325

Merged

ryantibs mentioned this pull request Nov 28, 2020

covidcast::covidcast_signal - substantial speed improvement possible #231

Closed

JedGrabman deleted the dev-covidcast-efficiency branch December 1, 2020 19:45

Batch requests when requesting multiple days of data #275

Batch requests when requesting multiple days of data #275

Uh oh!

Conversation

JedGrabman commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgsmob commented Nov 12, 2020

Uh oh!

capnrefsmmat commented Nov 12, 2020

Uh oh!

sgsmob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

capnrefsmmat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capnrefsmmat commented Nov 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JedGrabman commented Nov 12, 2020 •

edited

Loading

capnrefsmmat left a comment •

edited

Loading