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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Aug 14, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b642703
Improve handling for URL-based signals
rzats Aug 2, 2024
d4e2aee
Review fixes
rzats Aug 8, 2024
1481985
Review fixes
rzats Aug 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions src/api/EpiData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

export const currentEpiWeek = epidate.getEpiYear() * 100 + epidate.getEpiWeek();
export const currentDate = epidate.getYear() * 10000 + epidate.getMonth() * 100 + epidate.getDay();

Expand Down Expand Up @@ -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 }[]
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

> {
const url = new URL(ENDPOINT + `/covidcast_meta/`);
url.searchParams.set('format', 'json');
return fetchImpl<{ geo_type: string; signal: string; data_source: string }[]>(url).catch((error) => {
console.warn('failed fetching data', error);
return [];
});
return fetchImpl<{ geo_type: string; signal: string; data_source: string; time_type?: string }[]>(url).catch(
(error) => {
console.warn('failed fetching data', error);
return [];
},
);
}

export function importCDC({ locations, auth }: { locations: string; auth?: string }): Promise<DataGroup | null> {
Expand All @@ -182,18 +186,24 @@ export function importCOVIDcast({
}: {
data_source: string;
signal: string;
time_type?: 'day';
time_type?: string;
geo_type: string;
geo_value: string;
}): Promise<DataGroup | null> {
const title = `[API] Delphi CODIDcast: ${geo_value} ${signal} (${data_source})`;
const title = `[API] Delphi COVIDcast: ${geo_value} ${signal} (${data_source}) ${time_type}`;
rzats marked this conversation as resolved.
Show resolved Hide resolved
console.log(epiRange(firstEpiWeek.covidcast, currentEpiWeek));
rzats marked this conversation as resolved.
Show resolved Hide resolved
return loadDataSet(
title,
'covidcast',
{
time_type: 'day',
time_values: epiRange(firstDate.covidcast, currentDate),
},
time_type === 'day'
? {
time_type: 'day',
time_values: epiRange(firstDate.covidcast, currentDate),
}
: {
time_type: 'week',
time_values: epiRange(firstEpiWeek.covidcast, currentEpiWeek),
},
rzats marked this conversation as resolved.
Show resolved Hide resolved
{ data_source, signal, time_type, geo_type, geo_value },
['value', 'stderr', 'sample_size'],
);
Expand Down
6 changes: 5 additions & 1 deletion src/components/dialogs/dataSources/COVIDcast.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@
});

export function importDataSet() {
return importCOVIDcast({ data_source, signal, geo_type, geo_value });
return fetchCOVIDcastMeta().then((res) => {
const meta = res.filter((row) => row.data_source === data_source && row.signal === signal);
const time_type = meta[0].time_type || 'day';
rzats marked this conversation as resolved.
Show resolved Hide resolved
return importCOVIDcast({ data_source, signal, geo_type, geo_value, time_type });
});
}
</script>

Expand Down
1 change: 1 addition & 0 deletions src/data/data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ export const firstEpiWeek = {
quidel: 201535,
sensors: 201030,
nowcast: 200901,
covidcast: 202001,
};

// first available date for each data source
Expand Down
4 changes: 2 additions & 2 deletions src/deriveLinkDefaults.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,9 @@ export function initialLoader(datasets: ILinkConfig['datasets']) {
return Promise.all(resolvedDataSets).then((data) => {
const cleaned = data.filter((d): d is DataSet => d != null);
cleaned.forEach((d) => {
if (d.params && !Array.isArray(d.params) && d.params._endpoint && d.params.regions) {
if (d.params) {
// 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}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
add(d);
});
Expand Down