search

pharmaverse / blog

Blogging on the latest, greatest and most spectacular stuff happening around the pharmaverse
https://pharmaverse.github.io/blog/
Apache License 2.0
21 stars 9 forks source link

91 floating point #92

Closed StefanThoma closed 10 months ago

StefanThoma commented 11 months ago

Greta post @StefanThoma! Left some review comments.

Thanks for the thorough review, will implement this soon!

millerg23 commented 10 months ago

@StefanThoma looks really good, and you explain issue very well. Not sure if we should add in that we intend to implement a general option to hold significant digits (not done yet), so an easy way to update in one place for studies/companies, this was one of the main advantages of using signif() function. We can specify a reasonable default but ultimately users have control.

millerg23 commented 10 months ago

@bms63 I agree the post is excellent, but was wondering should we hold off releasing until we have a general option in place to hold the significant digits, and update the related function derive_var_anrind?? Think both are scheduled for this release?

StefanThoma commented 10 months ago

I'll try to get it ready by Wednesday, but I would not mind holding it off until the features are implemented in admiral. Or we can have it as a teaser and already publish now.

millerg23 commented 10 months ago

I'll try to get it ready by Wednesday, but I would not mind holding it off until the features are implemented in admiral. Or we can have it as a teaser and already publish now.

Yeah, good idea, publish when ready, and maybe add something about what we plan to do?

bms63 commented 10 months ago

I think we should publish we you have finished updates @StefanThoma. Can you link the issue in admiral and state that we are actively working on this issue.

I also think it would be nice to include the link to the blogpost in the changelog for admiral. I'll make a note of it in the admiral issue.

StefanThoma commented 10 months ago

I think we should publish we you have finished updates @StefanThoma. Can you link the issue in admiral and state that we are actively working on this issue.

Done.

StefanThoma commented 10 months ago

Hi @bms63 This is ready for review again :)

StefanThoma commented 10 months ago

@bms63 Feel free to publish!

bms63 commented 10 months ago

Publishing! I will make an announcement soon.

kaz462 commented 10 months ago

Just got a chance to read this blog - very well written, could be a nice reference when it comes to floating point in R :) Do you think it's necessary to clarify why 15 is chosen as a default value for signif_digits in the admiral option?

StefanThoma commented 10 months ago

Just got a chance to read this blog - very well written, could be a nice reference when it comes to floating point in R :) Do you think it's necessary to clarify why 15 is chosen as a default value for signif_digits in the admiral option?

Hi @kaz462 thanks for the positive feedback! :) Yeah that sounds like a good idea, also to include how users can change it. But tbh: I am not sure why exactly 15 digits were chosen. Why was that?

millerg23 commented 10 months ago

Just got a chance to read this blog - very well written, could be a nice reference when it comes to floating point in R :) Do you think it's necessary to clarify why 15 is chosen as a default value for signif_digits in the admiral option?

Hi @kaz462 thanks for the positive feedback! :) Yeah that sounds like a good idea, also to include how users can change it. But tbh: I am not sure why exactly 15 digits were chosen. Why was that?

It was really trial and error, the particular issue about 1.1 * 100 not being equal to 110 was addressed by using 16 significant digits on my Roche machine, so I went 1 less, as 15 still seemed like plenty of significant digits to not lose accuracy.