Make endpoint arg order consistent: geo, time

[dshemetov]

Fixes #24

[krivard]

This is a good idea but I suspect the argument ordering in this package was designed to match the old/existing delphi_epidata R client interface. Has this change been discussed with Tooling?

[dshemetov]

Ah, I will check in with them.

[brookslogan]

I forget where we landed on this. IF we go with this change, think there are some changes still required: e.g., covid_hosp_facility to be time then hospital_pks, and the covidcast_epidata interface would need to change as well.

But I'm not sure we want this order. I think time then geo is going to be a bit awkward paired with epiprocess, tsibble, (gtsibble if it materialized), which words things geo/key then time/index. And I think that most groups of endpoints have also been geo then time, which suggests that it might be the more natural order.

Regarding backwards compatibility: I'm mostly worried about people using flusight helper snippets like this one, which unfortunately do not call by name and use the two different orderings (covidcast and covidcast_epidata). I doubt but would like to know if people are using it already for other things. Any new users after this I wouldn't worry too much about: those that transition from covidcast pkg already have to reformat the time ranges; reordering isn't that much more hassle; and those that transition from delphi_epidata can probably deal with this small change (and may have to deal with larger hassles reformatting epiweeks/dates for epiranges).

Some options:

• No change --> awkward mix within epidatr, with other packages
• time, geo, don't change epiprocess --> awkward mix with other packages; breaks some helper snippets
• time, geo + reformat epiprocess? --> might be awkward if paired with tsibble (not common); breaks some helper snippets
• geo, time --> consistent with other packages; breaks some helper snippets

We should discuss at a tooling meeting soon.

[dshemetov]

I don't think I ever got much feedback on this. I'm not tied to a particular order, I just picked one because it seemed like a pretty arbitrary choice. I'm fine with (source_signal, geo, time) though, it makes sense.

I think it's worth enforcing consistency, even if it breaks some helper snippets. That's a one-time cost, whereas user frustration from argument inconsistency within and across packages would be a continual friction.

[brookslogan]

Informal poll here also suggests that geo before time is more natural. So unfortunately, I think things will need to be inverted here to follow a geo before time convention instead.

Consistency is definitely a major pro. I am a little nervous about breaking the only code snippets for this package we've really thrown out there to the modeling community to try using, especially with these types of error messages possible (this is the current "wrong" order):

epidatr::covidcast("hhs","confirmed_admissions_covid_1d","state","day","tx","20220101") %>% fetch_tbl()
#> Error in fetch_csv(epidata_call, fields) : Bad Request (HTTP 400).
epidatr::covidcast("hhs","confirmed_admissions_covid_1d","state","day","tx","20220101") %>% fetch_classic()
#> $epidata
#> data frame with 0 columns and 0 rows
#> 
#> $message
#> [1] "not a valid date: tx"
#> 
#> $result
#> [1] -1

although there's also this:

epidatr::covidcast("hhs","confirmed_admissions_covid_1d","state","day","tx",20220101)
#> Error in `check_string_param()`:
#> ! argument geo_values is not a string
#> Run `rlang::last_error()` to see where the error occurred.

which is maybe a little easier to debug. But I imagine we can do better. Perhaps a few early, ad-hoc checks to try to detect someone using the old order (e.g., using "state" for the time type or an epirange for the geo_values) and trying to set them straight + mentioning what version the ordering was swapped around in. And the snippet repo should be updated to require this version for the relevant code snippets [and fix its own breakage]. [Plus fetch_tbl should not be so cryptic; I don't know if we still get the $message and $result if it's a 400 or not though, but I imagine this is more of a separate issue. I'm hoping these other checks I've described would be something limited to a very small subset of endpoints / only covidcast.]

[krivard]

Perhaps a few early, ad-hoc checks to try to detect someone using the old order (e.g., using "state" for the time type or an epirange for the geo_values) and trying to set them straight + mentioning what version the ordering was swapped around in.

+1 on this in particular

[dshemetov]

Thanks for doing that poll Logan. Agree with having backwards compatibility for a few versions + a warning message.

If anyone wants to take over this PR, I would be happy with that. I'm not sure when I'll work on these clients again.

[brookslogan]

Looks good, all my questions are non-blocking.

question: should we also reorder the field info / columns for covidcast?

[brookslogan]

(but please sanity-check my minor changes)

[brookslogan]

Oh, wait...

issue: we don't have smart detection of people trying to use the old order. Should we add?

Also, Dmitry, could you please update flusight-helper-snippets to work with this update if it doesn't already?

[dshemetov]

I think we don't need to do smart detection here: the API server error codes should be enough

[dshemetov]

I also already fixed create_epidata_call in the other PR

[dshemetov]

I don't know what you mean about reodering column order / field info for covidcast. Could you link to code?