-
Notifications
You must be signed in to change notification settings - Fork 67
Add support for querying multiple signals #170
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
Conversation
@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. |
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? |
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. |
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. |
why not enforcing that a certain character such as |
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? |
as soon as we show small multiples of different signals in production, it will become handy. |
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. |
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 |
Yep, that was the original plan for this feature. Once #187 merges we'll
apply this one.
…On Fri, Aug 14, 2020 at 11:59 AM Samuel Gratzl ***@***.***> wrote:
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
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#170 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAI24CRSKOH5QRUGHMHU3RLSAVNMVANCNFSM4POYLAWA>
.
|
…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
28b9f8a
to
a57b61a
Compare
@melange396 check my work on a57b61a? tests pass but I want to make sure there's not a gotcha I didn't see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes cmu-delphi/covidcast-indicators#116