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

Handle analysis/define datasets w/o collection summaries #786

Merged
merged 14 commits into from
Jan 10, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
2 changes: 1 addition & 1 deletion app/scripts/components/analysis/define/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@
const onDatasetLayerChange = useCallback(
(e) => {
const id = e.target.id;
let newDatasetsLayers = [...(datasetsLayers || [])];

Check warning on line 295 in app/scripts/components/analysis/define/index.tsx

View workflow job for this annotation

GitHub Actions / lint

Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator
if (e.target.checked) {
const newDatasetLayer = allAvailableDatasetsLayers.find(
(l) => l.id === id
Expand Down Expand Up @@ -333,7 +333,7 @@
setAnalysisParam('datasetsLayers', cleanedDatasetsLayers);
// Only update when stac search gets updated to avoid triggering an infinite
// read/set state loop
}, [selectableDatasetLayers, setAnalysisParam]);

Check warning on line 336 in app/scripts/components/analysis/define/index.tsx

View workflow job for this annotation

GitHub Actions / lint

React Hook useEffect has a missing dependency: 'datasetsLayers'. Either include it or remove the dependency array

const notReady = !readyToLoadDatasets || !datasetsLayers?.length;

Expand Down Expand Up @@ -517,7 +517,7 @@
{datasetLayer.name}
<DataPointsWarning>
<CollecticonSignDanger />~
{datasetLayer.numberOfItems} data points
{datasetLayer.numberOfItems ? `${datasetLayer.numberOfItems} data points`: "Data temporarily unavailable"}
</DataPointsWarning>
</FormCheckableUnselectable>
))}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ interface UseStacSearchProps {
aoi?: FeatureCollection<Polygon> | null;
}

export type DatasetWithTimeseriesData = TimeseriesDataResult &
DatasetLayer & { numberOfItems: number };
export type DatasetWithCollections = TimeseriesDataResult & DatasetLayer;

export type DatasetWithTimeseriesData = DatasetWithCollections & { numberOfItems: number };

const collectionEndpointSuffix = '/collections';

Expand Down Expand Up @@ -64,14 +65,15 @@ export function useStacCollectionSearch({
enabled: readyToLoadDatasets
});

const datasetLayersInRange = useMemo(() => {
const [datasetLayersInRange, invalidDatasets] = useMemo(() => {
try {
return getInTemporalAndSpatialExtent(result.data, aoi, {
const [datasetWithSummaries, datasetWithoutSummaries] = getInTemporalAndSpatialExtent(result.data, aoi, {
start,
end
});
return [datasetWithSummaries, datasetWithoutSummaries];
} catch (e) {
return [];
return [[], []];
}
}, [result.data, aoi, start, end]);

Expand All @@ -89,11 +91,13 @@ export function useStacCollectionSearch({
);
}, [datasetLayersInRangeWithNumberOfItems]);

const unselectableDatasetLayers = useMemo(() => {
let unselectableDatasetLayers: DatasetWithTimeseriesData[] = useMemo(() => {
return datasetLayersInRangeWithNumberOfItems.filter(
(l) => l.numberOfItems > MAX_QUERY_NUM
);
}, [datasetLayersInRangeWithNumberOfItems]);

if (invalidDatasets.length) unselectableDatasetLayers = unselectableDatasetLayers.concat((invalidDatasets as unknown) as DatasetWithTimeseriesData[]);
hanbyul-here marked this conversation as resolved.
Show resolved Hide resolved

return {
selectableDatasetLayers,
Expand Down Expand Up @@ -156,14 +160,29 @@ function getInTemporalAndSpatialExtent(collectionData, aoi, timeRange) {
const collection = collectionData.find(
(c) => c.id === l.stacCol && stacApiEndpointUsed === c.stacApiEndpoint
);
return {

const datapoint: DatasetWithCollections = {
...l,
isPeriodic: collection['dashboard:is_periodic'],
timeDensity: collection['dashboard:time_density'],
domain: collection.extent.temporal.interval[0],
};

if(!collection.summaries || !!collection.summaries.length) {
// NOTE: Invalid data because collection does not include summaries
return datapoint;
}

return {
...datapoint,
timeseries: collection.summaries.datetime
};
});

return filteredDatasetsWithCollections;

const [collectionsWithSummaries, collectionsWithoutSummaries]: [DatasetWithCollections[], DatasetWithCollections[]] = filteredDatasetsWithCollections.reduce((result: [DatasetWithCollections[], DatasetWithCollections[]], d) => {
/* eslint-disable-next-line fp/no-mutating-methods */
d.timeseries ? result[0].push(d) : result[1].push(d);
return result;
},[[], []]);
return [collectionsWithSummaries, collectionsWithoutSummaries];
}
10 changes: 5 additions & 5 deletions app/scripts/components/analysis/results/chart-card.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@
if (status === 'loading') {
return 'Download will be available once the data finishes loading.';
}
if (!data.timeseries.length) {
if (!data.timeseries?.length) {
sandrahoang686 marked this conversation as resolved.
Show resolved Hide resolved
return 'There is no data to download.';
}
return '';
Expand Down Expand Up @@ -145,7 +145,7 @@
const onExportClick = useCallback(
(e: MouseEvent, type: 'image' | 'text') => {
e.preventDefault();
if (!chartData.data?.timeseries.length) {
if (!chartData.data?.timeseries?.length) {
return;
}

Expand Down Expand Up @@ -182,7 +182,7 @@

const chartDates = useMemo(
() =>
data?.timeseries.map((e) =>
data?.timeseries?.map((e) =>
dateFormatter(new Date(e.date), timeDensityFormat)
) ?? [],
[data?.timeseries, timeDensityFormat]
Expand Down Expand Up @@ -273,13 +273,13 @@
) : null}

{status === 'succeeded' ? (
data.timeseries.length ? (
data.timeseries?.length ? (
!activeMetrics.length ? (
<ChartCardNoMetric />
) : (
<Chart
ref={chartRef}
timeSeriesData={data.timeseries}
timeSeriesData={data.timeseries ||[]}

Check warning on line 282 in app/scripts/components/analysis/results/chart-card.tsx

View workflow job for this annotation

GitHub Actions / lint

Unnecessary conditional, value is always truthy
uniqueKeys={uniqueKeys}
colors={colors}
dates={chartDates}
Expand Down
2 changes: 1 addition & 1 deletion app/scripts/components/analysis/results/timeseries-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export interface TimeseriesDataResult {
isPeriodic: boolean;
timeDensity: TimeDensity;
domain: string[];
timeseries: TimeseriesDataUnit[];
timeseries?: TimeseriesDataUnit[]; // NOTE: Summaries on collections will not always be available
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make the type useful, we seem to need a new type. The components like chart-card has to have timeseries data, so it doesn't seem to reflect is actual usage to type timeseires as an optional. (Also the data with summaries and wo summaries having the same type is confusing, like this line : https://github.com/NASA-IMPACT/veda-ui/pull/786/files#diff-73d60515e81c1007b36e13ea2886f3b4e1457746381934d40af7802e39841022R182)

How about leaving the TimeseriesDataResult as it is, but making TimeseriesDataWithMissingSummaries (I just made the variable name up) and use this type explicitly for the data without summaries?

More food for thought: when the data is missing summaries, we don't seem to need any attributes from the collection?

Copy link
Collaborator Author

@sandrahoang686 sandrahoang686 Jan 4, 2024

Choose a reason for hiding this comment

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

I like the idea of being explicit but if we dont seem to need any attributes from the collection when summaries is missing from the data, then we can probably just use existing type DatasetLayer. Will make this change

}

// Different options based on status.
Expand Down
Loading