Improve handling for URL-based signals & support COVIDcast EpiWeeks #53

rzats · 2024-08-02T14:44:37Z

Closes #43, partially addresses #42:

URL-provided signals are given a title in the left-side panel based on their parameters (data source, signals, daily/weekly cadence etc.)
Added support for COVIDCast signals with a weekly cadence. This is based on the time_type specified in the meta endpoint, and works for both URL-provided signals as well as those selected in the signal picker.

rzats · 2024-08-02T14:45:54Z

src/api/EpiData.ts

@@ -34,7 +34,7 @@ export function epiRange(from: string | number, to: string | number): string {

 // find the current epiweek and date
 const date = new Date();
-const epidate = new EpiDate(date.getFullYear() + 1900, date.getMonth() + 1, date.getDate());
+const epidate = new EpiDate(date.getFullYear(), date.getMonth() + 1, date.getDate());


Currently epivis makes queries with 3924 set as the end year 🤔

Yeah, that has been a curious artifact. I'm not sure how it slipped through QA, but it has no real ill effect, and it let us track epivis-based requests to the API servers. It looks like it came from an older piece of javascript where it presumably did the right thing (¯\_(ツ)_/¯). I am leaning towards leaving it as-is until #38 is done (especially so we can see the effect in our logs of more eyes on epivis with the signal discovery app's release). Do you have strong feelings either way?

I believe this is already done by logging the referrer/origin in the delphi-epidata server? https://github.com/cmu-delphi/delphi-epidata/blob/dev/src/server/_common.py#L70-L71

If we want an even more reliable way to track API requests, a cleaner option is to
(a) start logging a certain custom HTTP header (like Delphi-Api-Origin) in the logging method I linked above;
(b) set that header in EpiVis, and in other apps that we would like to keep track of - like the client, for instance.

That server logging would do it if, as cmu-delphi/delphi-epidata#1386 (comment) mentions and #38 references, we update the Referrer-Policy for epivis. Im not suggesting that adding 1900y is "clean", but can we leave it there for now and note that it should come out with #38?

rzats · 2024-08-02T14:49:19Z

As a test for #42, use this link.

#43 can be tested by:

selecting Delphi COVIDcast, nchs-mortality, any signal, any state (e.g. "pa") in the signal picker
or using this link, which was generated by the Signal Documentation app.

melange396

This is great, and im impressed that you could get it done with this small of a diff!

melange396 · 2024-08-08T04:50:27Z

src/api/EpiData.ts

@@ -34,7 +34,7 @@ export function epiRange(from: string | number, to: string | number): string {

 // find the current epiweek and date
 const date = new Date();
-const epidate = new EpiDate(date.getFullYear() + 1900, date.getMonth() + 1, date.getDate());
+const epidate = new EpiDate(date.getFullYear(), date.getMonth() + 1, date.getDate());


Yeah, that has been a curious artifact. I'm not sure how it slipped through QA, but it has no real ill effect, and it let us track epivis-based requests to the API servers. It looks like it came from an older piece of javascript where it presumably did the right thing (¯\_(ツ)_/¯). I am leaning towards leaving it as-is until #38 is done (especially so we can see the effect in our logs of more eyes on epivis with the signal discovery app's release). Do you have strong feelings either way?

melange396 · 2024-08-08T04:50:31Z

src/api/EpiData.ts

@@ -150,13 +150,17 @@ export function loadDataSet(
    });
 }

-export function fetchCOVIDcastMeta(): Promise<{ geo_type: string; signal: string; data_source: string }[]> {
+export function fetchCOVIDcastMeta(): Promise<
+  { geo_type: string; signal: string; data_source: string; time_type?: string }[]


Was this filled in by your IDE because you use the .time_type member accessed in COVIDcast.svelte?

Why is the time_type marked "optional" with the question mark modifier? Is that just inherited from the importCOVIDcast() call signature definition? Or is it because the argument has a default value for time_type?

Was this filled in by your IDE because you use the .time_type member accessed in COVIDcast.svelte?

yes!

Was this filled in by your IDE because you use the .time_type member accessed in COVIDcast.svelte? Why is the time_type marked "optional" with the question mark modifier? Is that just inherited from the importCOVIDcast() call signature definition? Or is it because the argument has a default value for time_type?

Just inherited from that call.

src/api/EpiData.ts

src/components/dialogs/dataSources/COVIDcast.svelte

melange396 · 2024-08-08T04:50:48Z

src/deriveLinkDefaults.ts

          // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
-          d.title = `${d.params._endpoint} | ${d.params.regions} | ${d.title}`;
+          d.title = `${Object.values(d.params).join(' > ')} > ${d.title}`;


This is a definite improvement and works well for now, but i wouldnt call this a full solution for #42 ... It lacks clarity and doesnt reproduce the normal/typical LeftMenu for a COVIDcast-type signal.

instead of:
covidcast > doctor-visits > smoothed_cli > day > state > ar > value,
it would be preferable to have something more like:
covidcast > doctor-visits:smoothed_cli > state:ar
for brevity and expressiveness

(lets leave it the way it is for now in this PR and return to it later when other higher-priority PRs are out of the way)

I implemented this now, as it's fairly straightforward to modify the template. lmk what you think!

Looks great for now. We should keep #42 open to later support reproducing the full form dropdown with value/stderr/sample_size.

melange396

A couple little things, and then this'll be ready to go!

src/deriveLinkDefaults.ts

melange396 · 2024-08-09T23:40:36Z

src/api/EpiData.ts

@@ -34,7 +34,7 @@ export function epiRange(from: string | number, to: string | number): string {

 // find the current epiweek and date
 const date = new Date();
-const epidate = new EpiDate(date.getFullYear() + 1900, date.getMonth() + 1, date.getDate());
+const epidate = new EpiDate(date.getFullYear(), date.getMonth() + 1, date.getDate());


That server logging would do it if, as cmu-delphi/delphi-epidata#1386 (comment) mentions and #38 references, we update the Referrer-Policy for epivis. Im not suggesting that adding 1900y is "clean", but can we leave it there for now and note that it should come out with #38?

melange396 · 2024-08-09T23:41:48Z

src/deriveLinkDefaults.ts

          // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
-          d.title = `${d.params._endpoint} | ${d.params.regions} | ${d.title}`;
+          d.title = `${Object.values(d.params).join(' > ')} > ${d.title}`;


Looks great for now. We should keep #42 open to later support reproducing the full form dropdown with value/stderr/sample_size.

rzats · 2024-08-12T21:16:58Z

@melange396 updated this!

melange396

nice work!

Improve handling for URL-based signals

b642703

rzats commented Aug 2, 2024

View reviewed changes

rzats requested a review from melange396 August 2, 2024 14:49

melange396 requested changes Aug 8, 2024

View reviewed changes

Review fixes

d4e2aee

rzats requested a review from melange396 August 8, 2024 12:58

melange396 changed the title ~~Improve handling for URL-based signals~~ Improve handling for URL-based signals & support COVIDcast EpiWeeks Aug 9, 2024

melange396 requested changes Aug 9, 2024

View reviewed changes

Review fixes

1481985

rzats requested a review from melange396 August 12, 2024 21:16

rzats mentioned this pull request Aug 12, 2024

Make epivis API requests more visible #38

Open

melange396 approved these changes Aug 14, 2024

View reviewed changes

melange396 merged commit 8baf276 into dev Aug 14, 2024
6 checks passed

melange396 deleted the rzatserkovnyi/url-signals branch August 14, 2024 14:11

melange396 mentioned this pull request Aug 22, 2024

Release v2.1.3 #56

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling for URL-based signals & support COVIDcast EpiWeeks #53

Improve handling for URL-based signals & support COVIDcast EpiWeeks #53

rzats commented Aug 2, 2024 •

edited

Loading

rzats Aug 2, 2024

melange396 Aug 8, 2024

rzats Aug 8, 2024

melange396 Aug 9, 2024

rzats commented Aug 2, 2024

melange396 left a comment

melange396 Aug 8, 2024

melange396 Aug 8, 2024

rzats Aug 8, 2024

melange396 Aug 8, 2024

rzats Aug 8, 2024

melange396 Aug 9, 2024

melange396 left a comment

melange396 Aug 9, 2024

melange396 Aug 9, 2024

rzats commented Aug 12, 2024

melange396 left a comment

Improve handling for URL-based signals & support COVIDcast EpiWeeks #53

Improve handling for URL-based signals & support COVIDcast EpiWeeks #53

Conversation

rzats commented Aug 2, 2024 • edited Loading

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

rzats commented Aug 2, 2024

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

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

rzats commented Aug 12, 2024

melange396 left a comment

Choose a reason for hiding this comment

rzats commented Aug 2, 2024 •

edited

Loading