Skip to content
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

Add support for querying multiple signals #170

Merged
merged 2 commits into from
Sep 4, 2020

Conversation

krivard
Copy link
Contributor

@krivard krivard commented Jul 30, 2020

Fixes cmu-delphi/covidcast-indicators#116

  • backwards-compatible signal/signals parameter
  • optional 'format' parameter for retrieving results as a tree (preferred by Viz) or as a flat list (backwards-compatible); defaults to flat list
  • client support: coffee, js, R, py
  • integration tests to check multi-signal queries and queries for tree format

@krivard
Copy link
Contributor Author

krivard commented Jul 30, 2020

@capnrefsmmat can you check my work in the raw R client? Backwards-compatibility is not something I've had to deal with before in R.

@sgratzl
Copy link
Member

sgratzl commented Jul 31, 2020

looking good, just to clarify: the tree format is

{
  signal1: [...],
  signal2: [...],
}

for me, I would call it more a group by signal. One could do a similar things for every parameter that allows multiple values.

general question: this PR allows me to query multiple signals of the same data source? how about multiple signals distributed in different data sources?

@capnrefsmmat
Copy link
Contributor

The R client change looks fine -- it should support the old syntax and anyone using named arguments. I'm not sure how jsonlite will parse the tree format, but as long as the default format is unchanged from before, current users should not be surprised.

@krivard
Copy link
Contributor Author

krivard commented Aug 3, 2020

@sgratzl

how about multiple signals distributed in different data sources?

Since we don't enforce unique signal names, what we'd really want for that is to accept (source, signal) pairs, and that's a larger architectural shift than we're really prepared for just now.

@sgratzl
Copy link
Member

sgratzl commented Aug 4, 2020

why not enforcing that a certain character such as : doesn't appear and that you have a combined unique signal id: datasource:signal

@krivard
Copy link
Contributor Author

krivard commented Aug 4, 2020

That would probably be a fine way to handle it in the clients, but I'm more concerned about the query architecture just now. How important is this to you, should we wait to merge this PR until we get full (source, signal)* support working?

@sgratzl
Copy link
Member

sgratzl commented Aug 4, 2020

as soon as we show small multiples of different signals in production, it will become handy.

@krivard
Copy link
Contributor Author

krivard commented Aug 4, 2020

Followed up with @sgratzl offline: Small multiples sends 12 API requests (one query per source-signal pair) in the default view, but network time for these queries is less than 10% of the page render time, largely due to browsers parallelizing network requests.

Since small multiples is the major use case for source-signal pairs, and it's not a responsiveness issue, we'll hold off on source-signal pair support for now.

@sgratzl
Copy link
Member

sgratzl commented Aug 14, 2020

another use case which involves more data is when querying for average / count / average cumulative / count cumulative of cases / deaths. since they are all coming from the same data source this PR would save a lot

@krivard
Copy link
Contributor Author

krivard commented Aug 14, 2020 via email

…ators#116

* backwards-compatible signal/signals parameter
* optional 'format' parameter for retrieving results as a tree (preferred by Viz) or as a flat list (backwards-compatible); defaults to flat list
* client support: coffee, js, R, py
* integration tests to check multi-signal queries and queries for tree format
* Correctly fail files with bad headers
* Streamline files with bad rows: fail without needing to hit the database
@krivard krivard force-pushed the feature/cvc-multi-signal-queries branch from 28b9f8a to a57b61a Compare September 4, 2020 13:44
@krivard krivard requested review from melange396 and removed request for capnrefsmmat September 4, 2020 13:45
@krivard
Copy link
Contributor Author

krivard commented Sep 4, 2020

@melange396 check my work on a57b61a? tests pass but I want to make sure there's not a gotcha I didn't see

Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@krivard krivard merged commit 530612d into cmu-delphi:main Sep 4, 2020
@krivard krivard deleted the feature/cvc-multi-signal-queries branch June 5, 2023 17:58
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.

[covidcast] Add support for querying multiple signals
5 participants