-
Notifications
You must be signed in to change notification settings - Fork 34
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
476 analysis comparison; COUNTRY=myanmar #880
Conversation
Build succeeded and deployed at https://prism-880.surge.sh |
frontend/src/utils/analysis-utils.ts
Outdated
@@ -111,6 +111,7 @@ const operations = { | |||
const ceil = sortedValues.length / 2; | |||
return (sortedValues[floor] + sortedValues[ceil]) / 2; | |||
}, | |||
intersect_percentage: (data: number[]) => mean(data), |
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 am not sure about calculation required for intersect_precentage. Let me know if we need to change it.
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.
Hmm I think we need to error out in this case or at least raise a warning. With just the percentage we can't give an accurate number here from the frontend. But this shouldn't be triggered atm as the analysis is run by the backend, no?
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 think I don't really understand what is the usage of this operator. I can see it is used by the loadFeaturesClientSide function that is called by the fetchImpactLayerData. I was thinking it was used to display the layer with the selected statistic but it seems not. If I change with "intersect_percentage: () => null" it continues working fine. I change this line because of typescript but I don't know the usage/impact and what to do with it.
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.
So if I recall correctly this is used to make the analysis calculations locally, if you don't have access to a stats API.
It is less relevant now but at the beginning, some deployments wanted to be "independent" from the API.
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.
ok understood ! I change to raise an error if trying to calculate intersect_percentage from client side then
…p into 476-analysis-comparison
Hi @wadhwamatic, When we query the stats with: intersect_comparison | ">=3" We get in response for Thegon TS_PCODE: MMR008005
|
Yes this is an edge case with the API. That particular grid is mostly empty and the API returns the percentage of pixels with the correct value. But it cannot count empty pixels. So what it means is that 100% of the grid pixels available in this area have a value of 3. The API should work well with normal rasters already. I'll look into a way to count/add empty pixels as well. |
The API edge case should be resolved by #893 |
@jbperidypathtech copy-pasting from Slack to share context: After discussing with Amit, we would like you to investigate the following behavior, if it’s not too tricky to handle on the frontend (I know most of the components are built with one metric in mind so it might be 🤷 )
In addition, I noticed that the table ordering is not working correctly for percentages atm |
@jbperidypathtech - this is looking good. There's a couple of things I'll need to sync with @ericboucher on and will share more feedback afterwards. In the meantime, I tried running an analysis on flood extent, and then another one on rainfall. The tooltip label still referred to flood extent: |
@jbperidypathtech - I just sync'd with @ericboucher on this and agreed that a couple of things should be added here:
|
Hi @wadhwamatic, |
# and filter out properties that have "no" intersection | ||
# by setting a limit at 0.005 (0.5%). |
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.
@wadhwamatic As it happened there were actually very few admin boundaries with zero
value, but very small percentages. I set an arbitrary limit of 0.5%. Let me know what you think.
…prism-app into 476-analysis-comparison
This reverts commit 4069a2d.
This fixes issue 476.
How to test the feature:
Notice: I tried to improve the responsivity of the app when we change the statistic (to max to min for example) because values are available in the front. But there is a lot of thing to change and I am not sure of the impacts. So I think it should be great to create new ticket to globally improve the left menu UX to auto-recalculate statistic table and map when any configuration value in the left menu change.
Screenshot/video of feature: