trisomy-21-bmi-rendering-issue-fix #96
Conversation
| @@ -360,6 +362,11 @@ function updateCoordsOfExtremeValues( | |||
| if (extremeValues.highestY < d.y) { | |||
| extremeValues.highestY = d.y; | |||
| } | |||
| // this is necessary because in the BMI dataset (esp Trisomy-21), the values for Y ramp up to infinitity towards the end of the dataset | |||
| // this is a hack to prevent the chart from scaling to infinity - see discussion in #93 about the nature of SDS calculation when L is 0 or negative | |||
| if (extremeValues.highestY > 500){ | |||
There was a problem hiding this comment.
I was a bit confused how this works so added some logging and I think it relies on d.y being null after the run of increasing values? So ultimately it resets highestY to then pick up a more reasonable value when processing the next points?
My first thought was why not clamp with a max here but I guess this way the plot ends up scaled to fit the actual data.
I'm wondering would this break if we lost the nulls in a future chart-coordinates update though.
There was a problem hiding this comment.
The presence of nulls allows us to plot values against x even when there is no reference data - this for example was a request from the neonatologists who wanted the facility to plot weights on preterm infants from 22 weeks gestation where there is no reference data. There I added extra null values to the reference data to allow this. I am afraid I forget now why extra nulls were added here, but not all datasets end at the same age - for example girls head circumference ends at 17y, while the boys ends at 18y and weight and height in UK90 go all the way up to 23 y.
I didn't actually write this code - @chvanlennep is the brain behind it - but I think the idea is to plot a measurement, even if it is higher than the visibile centile data. The extra lines I added were to filter out anything over 500 as this is pretty much impossible anyway and matches the thresholds (domains, as they are known in VictoryChart speak) already set by default.
If you are happy with this I will merge into live and push a patch - let me know and I will go ahead. I am happy to raise an issue in the internal dev chat about chart domains if you wish on the forum? I am not wedded to this methodology and am happy to change it if you feel there is a better approach.
There was a problem hiding this comment.
Cool thanks for the explanation. Happy to merge this, I think we can go with a "three strikes" approach to this code - once we've added the third hack or special case we can look at how to make it more resilient based on the reports we get from users
|
I've run this up locally and it does fix the chart rendering for me but I've added a comment - I'm wondering how resilient this change will be to future chart coordinates updates. Also @eatyourpeas please add a before and after screenshot for visual changes :) |
|
of course. Sorry, before and after images were added to the issue, but not the PR, so here it is. |
Overview
Bug identified by @princepsivannhs where the BMI dataset for Trisomy-21 contains values that run into the thousands. This is function of the LMS calculation such that when the value of L is negative (see discussion in the issue #93), z rapidly rises to infinity.
To overcome this, visible data is filtered exclude higher values of y before plotting.
Code changes
getDomainsAndData.tsincludes a conditional to filter out values > 500.Documentation changes (done or required as a result of this PR)
See issue #93 for discussion
Related Issues
Closes #93
Mentions
Thank you to @princepsivannhs for identifying, @statist7 for clarifying the maths and @mbarton to debugging and creating a new story.