Add analysis data request with AOIs #699
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Amazing to see this working! 😍 Probably premature comment - the AOI is very hard to see e.g. on top of GEOGLAM...
|
| const dataset = useAtomValue(datasetAtom); | ||
| const datasetStatus = dataset.status; | ||
|
|
||
| const [, setAnalysis] = useTimelineDatasetAnalysis(datasetAtom); |
There was a problem hiding this comment.
You could have useTimelineDatasetAnalysis only return a setter (useSetAtom) and simplify this:
| const [, setAnalysis] = useTimelineDatasetAnalysis(datasetAtom); | |
| const setAnalysis = useTimelineDatasetAnalysis(datasetAtom); |
There was a problem hiding this comment.
that's a good point. I was trying to be consistent with the other useTimelineDataset*, like useTimelineDatasetVisibility or useTimelineDatasetSettings where they return getters and setters. You think it is not needed?
There was a problem hiding this comment.
The consistency argument is a reasonable one, but in that case why not just returning the atoms themselves?
export function useTimelineDatasetAnalysisAtom() {
const analysisAtom = ...
return analysisAtom;
}
| const [, setAnalysis] = useTimelineDatasetAnalysis(datasetAtom); | |
| const setAnalysis = useSetAtom(useTimelineDatasetAnalysisAtom(datasetAtom)); |
There was a problem hiding this comment.
I was trying to avoid this function train and make the hook usage more direct. Nevertheless I'm not set on this format and I'm not opposed to changing it. Do you have a preference?
There was a problem hiding this comment.
+1 on @nerik 's comment that we need to put a guardrail to prevent excessive amount of requests.
When I was checking, Nightlight dataset was returning errors on some of the items. (Not sure if it is something that can be reproduced consistently or a hiccup. Bonus point if you can shed an insight to test without depending on actual response! ) And a dataset list item doesn't reflect the error, gets stuck at the loading state. (As far as I can read, it seems error state gets washed out from the other request's loading state?)
|
@hanbyul-here You unearthed quite an interesting bug. I think I fixed the problem and now when there's an error you'll get the proper message and the loading will stop. You'll also be able to retry the analysis which was not yet implemented 😅. As for Nightlights: it sometimes errors seemingly randomly. It seems that if there are a lot of requests it errors (although we're throttling at 15 simultaneous). Perhaps it has to do with api load or something. @ividito any debugging we can do here? |
Allow analysis runs to be controlled on a dataset level. This means having an analysis run per dataset as well as having a way to dequeue specific requests.
danielfdsilva
force-pushed
the
feature/analysis-requests
branch
from
b679001 to
d86ff7e
Compare
|
I am a bit confused that the pr is still in draft state. But giving approval since the changes I requested were made. 👍 |
|
@hanbyul-here Thank you. It is Draft since still need to address the message with @ricardoduplos . But the functionality can be reviewed :) |
I looked into this and it looks like a handful of nightlights "regions" will cause excessive loading times, which can cascade into the service timing out. I think the newest version of the preview handles this a bit better - I still see longer loading times in some areas, but it's no longer causing an actual error for me. This is probably something we can correct on the backend - I'll make an issue to track this in that repo. |
|
@ricardoduplos Messages implemented according to the design. Please have a look! |
Implements the analysis flow, using the aois drawn on the map, requests data and displays it on the chart.
Also adds support for each dataset to have independent customizable metrics.