-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Support custom time interval #1560
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
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
manually validated. other than some comments, lgtm.
throw new Error( | ||
`Missing time interval for periodic dataset [${datasetId}]` | ||
); | ||
} |
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.
i'd move above line 101 and throw right away before having to do that utc date conversion
`Unsupported time interval [${timeInterval}] on dataset [${datasetId}]` | ||
); | ||
} | ||
} |
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.
unfortunately none of these good informative error messages will see light and be ignored due to the error being caught and a different error message will be used instead of the original :(. https://github.com/NASA-IMPACT/veda-ui/blob/main/app/scripts/components/exploration/hooks/use-stac-metadata-datasets.ts#L73-L80. To make this PR more useful, can you update to make the general error message and the actual error message you've added to this logic display? And we need to come up with a better error handling system in veda-ui ...
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.
I have some questions and it would be great to have some tests for the new date handling method.
This should not be a blocker, but to leave a note: I think we are actually resolving the temporal layer in two places, first in fetchStacDatasetById
and then in resolveLayerTemporalExtent
. fetchStacDatasetById
is doing much more than fetching the data (introduced by precedent handlings of timestamps for different datasets). We probably want to move this logic to somewhere likereconcileQueryDataWithDataset
in data-utils.
I also wonder if we need to consider a better UI for custom time intervals - this is related to my inline question (#1560 (comment)) All other UIs are kind of depending on TimeDensity attribute. For example, will there be TimeDensity attribute for the data that has P3M period or P2H- if so, what would be it? We are shimming the density attribute with DAY
now, so it would be either the user will get too granular grid for their data (P3M case), or not granular enough UI to select the time they want(P2H).
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.
One issue worth being aware is that the date that a user sees, won't be the date of the data. (Data request will fetch the most relevant date, but UI shows the selected date - related part:
const relevantDate = useMemo( |

### Description of Changes Adds support for wms datasets ### Notes & Questions About Changes Split from #1445 Instead of defining it as "arcgis" dataset, using the generic "wms" terminology because any other wms dataset/layer can be visualized the same way. This also includes the changes from #1560. ### Validation / Testing Working link: https://deploy-preview-1565--veda-ui.netlify.app/exploration?search=&datasets=%5B%7B%22id%22%3A%22esi_4wk_layer%22%2C%22settings%22%3A%7B%22isVisible%22%3Atrue%2C%22opacity%22%3A100%2C%22analysisMetrics%22%3A%5B%7B%22id%22%3A%22mean%22%2C%22label%22%3A%22Average%22%2C%22chartLabel%22%3A%22Average%22%2C%22themeColor%22%3A%22infographicB%22%7D%2C%7B%22id%22%3A%22std%22%2C%22label%22%3A%22St+Deviation%22%2C%22chartLabel%22%3A%22St+Deviation%22%2C%22themeColor%22%3A%22infographicD%22%7D%5D%2C%22scale%22%3A%7B%22min%22%3A0%2C%22max%22%3A1%7D%7D%7D%5D&taxonomy=%7B%7D&date=2001-01-23T06%3A00%3A00.000Z&dateRange= Any datasets with the "arcgis" search query is relevant to this pr.
Related Ticket: #1463
Description of Changes
To support custom time steps (not just 1 day, 1 month, 1 year), we're adding a
dashboard:time_interval
key to STAC collections. This is in the ISO time duration format.This PR adds support for this new key while still being backward compatible.
Adds a function to generate valid dates based on the start/end/and the various time intervals.
Notes & Questions About Changes
N/A
Validation / Testing
dashboard:time_interval
value and that the layers load correctlyThis preview from this other PR shows that it works for a P7D dataset: https://deploy-preview-1565--veda-ui.netlify.app/exploration?search=&datasets=%5B%7B%22id%22%3A%22esi_4wk_layer%22%2C%22settings%22%3A%7B%22isVisible%22%3Atrue%2C%22opacity%22%3A100%2C%22analysisMetrics%22%3A%5B%7B%22id%22%3A%22mean%22%2C%22label%22%3A%22Average%22%2C%22chartLabel%22%3A%22Average%22%2C%22themeColor%22%3A%22infographicB%22%7D%2C%7B%22id%22%3A%22std%22%2C%22label%22%3A%22St+Deviation%22%2C%22chartLabel%22%3A%22St+Deviation%22%2C%22themeColor%22%3A%22infographicD%22%7D%5D%2C%22scale%22%3A%7B%22min%22%3A0%2C%22max%22%3A1%7D%7D%7D%5D&taxonomy=%7B%7D&date=2001-01-23T06%3A00%3A00.000Z&dateRange=