statist7 / LMSgrowth2

MIT License
2 stars 2 forks source link

Add centile tab #26

Closed giordano closed 5 years ago

giordano commented 5 years ago

This is not complete yet and needs some more interaction between different components, but I think I implemented most of the features in #8.

Here are a couple of screenshots to see how it appears now:

centiles1 centiles2

statist7 commented 5 years ago

That looks exciting – I’m keen to try it.

Best wishes, Tim

statist7 commented 5 years ago

It looks good.

The age range needs to reflect the available data – with who06 the data stop at age 5 yet the table extends to age 23 with lots of NAs.

I’d prefer fewer significant digits in the output. Is there somewhere central that controls this?

Is there a simple way to make the 2 in kg/m^2 a superscript?

Best wishes, Tim

statist7 commented 5 years ago

Sorry, I missed this. If with who06 I select leg length there are two neat error messages in the dashboard plus a whole string of errors in the console.

Best wishes, Tim

giordano commented 5 years ago

Yes, we want to make the age ranges and available measurements automatically adapt to the chosen reference data. Pull request #27 should add this feature.

statist7 commented 5 years ago

Excellent!

giordano commented 5 years ago

I think this is now ready for review

statist7 commented 5 years ago

Overall this looks really good - I love the flexibility of changing the measurement, age and sex.

statist7 commented 5 years ago

I agree with @tamuri about the value of downloading the data frame, perhaps with user-specified delimiters.

giordano commented 5 years ago

I have implemented most of the changes suggested, but I still have to do the download. This is how it looks like now: image

I removed the function set_custom_centiles. The problem is that not making the table change requires all the decimal digits. In case we want to go back, we just need to revert commit 3e936a6 (and probably adapt it a little bit). The list of custom centiles is space-separated now, just like the same list in the "Density" tab.

statist7 commented 5 years ago

Looks good. Some minor comments.

The space between centiles has too many decimal places - I suggest 2. It needs to say it's in SDS units.

The number of centiles gives an error if the box is empty, and similarly for the centiles/SDS box.

Entering p for the number of centiles where p < 1 gives 2-p centiles - curious! I recognise this overrides the integer counter.

The units for the custom centiles can be adjacent to each other, like the age units, and they don't need the 'custom'.

Entering 0 or 100 in the custom centiles box correctly gives ±Inf, but a value < 0 or > 100 gives an error. The custom SDS box has no such problems as it covers the real line.

Can the centile options be moved down to below the sex and age options?

statist7 commented 5 years ago

A further point. The default reference is UK90 from 0 to 23 years by 6 months. But if one switches to UK-WHO preterm it keeps the same age units, resulting in a table with just one row at age 0 years. There is no way of knowing what the reference's age range is, other than by experimenting.

Switching references needs somehow to display the first and last available age, and if this requires the age unit to change then so be it. Alternatively the age range could be displayed separately.

giordano commented 5 years ago

The space between centiles has too many decimal places - I suggest 2. It needs to say it's in SDS units.

I've just used the same default value as the Excel plugin, that is 2/3 = 0.666666.... Maybe we should a different default value like 0.5?

The number of centiles gives an error if the box is empty, and similarly for the centiles/SDS box.

I think the same happens whenever a text input field is empty, an error must be thrown because it is an error. If there is no input we cannot make it up for the user. Anyway, I'm looking into improving the error message, I agree that often they're not very clear.

Entering p for the number of centiles where p < 1 gives 2-p centiles - curious! I recognise this overrides the integer counter.

Note that all these fields have a minimum and maxim allowed value, you can see if you use the scroll wheel of the mouse or the up and down arrows of the keyboard. This field in particular accepts integers from 1 to 30, but then the user can write whatever they want. I'm going to add an additional validation -- it's annoying that Shiny doesn't do this since it already knows the allowed range.

The units for the custom centiles can be adjacent to each other, like the age units, and they don't need the 'custom'.

Ok, I'm going to fix this!

Entering 0 or 100 in the custom centiles box correctly gives ±Inf, but a value < 0 or > 100 gives an error. The custom SDS box has no such problems as it covers the real line.

I'm using qnorm(value / 100) to get the SDS, this gives NA if value is less than 0 or larger than 100. Getting NA is correct. I'm going to improve the error message also here.

Can the centile options be moved down to below the sex and age options?

Sure, no problem!

statist7 commented 5 years ago

The space between centiles has too many decimal places - I suggest 2. It needs to say it's in SDS units.

I've just used the same default value as the Excel plugin, that is 2/3 = 0.666666.... Maybe we should a different default value like 0.5?

No, the two-thirds is important. But can you not display it to say 3 or 4 decimal places rather than the current 15 ? It will make no difference to the centiles.

Everything else looks fine.

giordano commented 5 years ago

I've just pushed a commit that writes in the "Range" header the allowed range. I hope that in this way it's more clear for the user what value they can enter. It looks like this:

image

Note that both From and To fields already had a minimum and maximum allowed value, which dynamically depend on the age unit and the reference data selected. Inserting a value too small or too large (or leaving the field empty) would show a tooltip when hovering the mouse over the field.

statist7 commented 5 years ago

That works well. I put in too small an age and it gave the error 'The minim allowed value..." - can that be minimum ! ?

giordano commented 5 years ago

That works well. I put in too small an age and it gave the error 'The minim allowed value..." - can that be minimum ! ?

Yes, that was a typo, thanks for pointing it out!

giordano commented 5 years ago

It is possible that some growth references will have data for only one sex. I wanted to check that this will be handled tidily.

That's good to know, thanks for pointing it out! In the data set we're working on now 1 stands for male and 2 for female, but in sitar there is a different mapping: 'm' for male, and 'f' for female. Does this still hold in the case of single-sex dataset? Or, for example the only sex available could be identified by 1 even in the case it's female?

I'll probably create a global variable holding the available sexes.

Note also that sitar doesn't do any validation of the input, so that

data(uk90)
zs <- -4:4*2/3 # z-scores for centiles
ages <- 0:12/4 # 3-month ages
sitar::LMS2z(ages, as.matrix(zs), sex = 'z', measure = 'wt', ref = uk90, toz = FALSE)

returns something (I think the male data) anyway even if sex = 'z' is not a meaningful sex.

statist7 commented 5 years ago

The coding of sex is tricky, as you point out. I've improved LMS2z so it properly checks the sex, and it now gives an error with your example.

LMS2z accepts the following values for sex: 1 M B T for male, and 2 F G for female, and anything else is set to NA. Note that T is for TRUE to match F for FALSE (also Female). It returns 1 for male and 2 for female.

I will ensure in the future that any female-only references are coded 2.