Enhancement: Units edition results #146
Conversation
Gero1999
added
help wanted
Merge branch 'main' into enhancement/units-edition-results # Conflicts: # R/export_cdisc.R # inst/shiny/tabs/nca.R # inst/shiny/tabs/outputs.R
|
Hello team @m-kolomanski @Gotfrid @js3110 ! I have a small technical problem with this When the package is installed through non-Windows OS, the installation through CRAN with There are many potential solutions to this, as the only affected things are the
Let me know what you think of them or if you could come up with a better solution and I will try to implement it! |
|
Hi @Gero1999, use whatever dependencies you need to use - there is no need to re-invent the wheel if ready solutions are available, and we are not the first to deal with system dependencies, so do not worry :) In terms of installing the package itself, if installing a dependency (in that case In terms of github actions / workflows, we can install the dependency on the machine that runs the checks, I will get on that. |
|
I agree with @m-kolomanski, adding the dependency is the best path. Writing your own unit handling will likely be error-prone. (I've done it a few times in the past, and it always has lots of bugs the first time around.) The units package has the side-benefit that all standard unit conversions should be handled for you. |
|
I think the feature itself is ready for review. The other conflicts will hopefully be solved once the workflow incorporates the dependency mentioned. Let me know if so far this convince you or you can think of improvements: Feat characteristics
|
Gero1999
requested review from
m-kolomanski and
js3110
and removed request for
js3110
|
Agreed with everything mentioned above regarding the |
inst/shiny/modules/units_table.R
Outdated
| new_res_nca <- reactiveVal(NULL) | ||
| observeEvent(input$save_units_table, { | ||
|
|
||
| # Make sure there are no missing entries (no NA in conversion factor) | ||
| if (any(is.na(modal_units_table()$`Conversion Factor`))) { | ||
|
|
||
| invalid_entries <- modal_units_table() %>% | ||
| filter(is.na(`Conversion Factor`)) %>% | ||
| mutate(entry = paste0(Parameter, " (", Analytes, ")")) %>% | ||
| pull(entry) | ||
|
|
||
| showNotification( | ||
| paste0("Please, make sure to use only recognised convertible units in `Custom Unit`", | ||
| "(i.e, day, hr, min, sec, g/L).", | ||
| " If not, introduce yourself the corresponding `Conversion Factor` value in: ", | ||
| paste(invalid_entries, collapse = ", ")), | ||
| duration = NULL, | ||
| closeButton = TRUE, | ||
| type = "warning" | ||
| ) | ||
| return() | ||
| } | ||
|
|
||
| # Tranform the modal units table back to the original one | ||
| modal_units_table <- modal_units_table() %>% | ||
| dplyr::mutate(Analytes = strsplit(Analytes, ", ")) %>% | ||
| tidyr::unnest(Analytes) %>% | ||
| dplyr::rename(ANALYTE = `Analytes`, | ||
| PPTESTCD = `Parameter`, | ||
| PPORRESU = `Default unit`, | ||
| PPSTRESU = `Custom unit`, | ||
| conversion_factor = `Conversion Factor`) | ||
|
|
||
| # Save the modified units table in my data object | ||
| mydata <- mydata() | ||
| mydata$units <- modal_units_table | ||
| mydata(mydata) | ||
|
|
||
| # If there are already results produced, make sure they are also adapted | ||
| if (!is.null(res_nca())) { | ||
| res_nca <- res_nca() | ||
| res_nca$data$units <- modal_units_table | ||
| res_nca$result <- res_nca$result %>% | ||
| dplyr::select(-PPSTRESU, -PPSTRES) %>% | ||
| dplyr::left_join( | ||
| modal_units_table, | ||
| by = intersect(names(.), names(modal_units_table)) | ||
| ) %>% | ||
| dplyr::mutate(PPSTRES = PPORRES * conversion_factor) %>% | ||
| dplyr::select(-conversion_factor) | ||
|
|
||
| new_res_nca(res_nca) | ||
| } | ||
| # Close the module window once all saving actions are done | ||
| removeModal() | ||
|
|
||
| }) | ||
|
|
||
| return(reactive(new_res_nca())) | ||
| }) |
There was a problem hiding this comment.
suggestion: This also could be wrapped into single reactive() expression and returned directly. You can use bindEvent to bind a reactive expression to a single specific event, instead of recalculating it every time some dependency change (it is basically like wrapping it in isolate().
reactive({
# code for pepraring the results
removeModal()
res_nca
})There was a problem hiding this comment.
issue: There are a lot of side-effect in this module and they seem to not be consistent. You are modifying mydata() object directly in this module, but instead of also modifying res_nca() directly you return it as a reactive, and then assign the value in anca.R file (Line 37). There is nothing inherently wrong with mutating the state of the object directly, but it gets confusing as the complexity grows and might be hard to debug later on. Please make sure all side-effects of the module are desired and documented.
inst/shiny/tabs/nca.R
Outdated
| units_table_server("units_table_preNCA", mydata, reactive(NULL), params_to_calculate) | ||
| res_nca2 <- units_table_server("units_table_postNCA", mydata, res_nca, params_to_calculate) | ||
| observeEvent(res_nca2(), res_nca(res_nca2())) |
There was a problem hiding this comment.
question: So if I understand that correctly, we are using units_table_preNCA module to modify only mydata object, but use units_table_postNCA to modify both mydata and res_nca? This is a bit confusing and relates to my previous comment about side-effects. Seems like a single instance of the module should be able to handle both cases (and just return NULL if res_nca is not available).
| @@ -371,8 +384,8 @@ observeEvent(input$nca, { | |||
| }) | |||
|
|
|||
| # run the nca upon button click | |||
|
|
|||
| res_nca <- eventReactive(rv$trigger, { | |||
| res_nca <- reactiveVal(NULL) | |||
There was a problem hiding this comment.
issue: res_nca is referenced on line 307, but only defined on line 387. This does not cause any technical problems because of the even structure of the program, but is bad practice nevertheless - this should be higher. Considering the importance of that data, I would consider putting this definition on top of the module.
style: m-kolomanski suggested changes Co-authored-by: Mateusz Kołomański <[email protected]>
Issue
Users would like to be able to choose the units in which the results parameters are displayed.
Closes #113
Description
Sometimes user might be interested in changing the standard format of units provided by the app. In one that this may happen frequently is for example clearance (cl.obs), where current units always make very small values (~0) that can confuse when the rounding is used in the summary statistics.
Definition of Done
How to test
How to test features not covered by unit tests.
Contributor checklist
System to change units in parameters works well before NCA
System to change units in parameters works well after NCA
Outputs post NCA still work fine and adapt well to new units (i.e, boxplots)
Code passes lintr checks
Code passes all unit tests
New logic covered by unit tests
New logic is documented
Notes to reviewer
Anything that the reviewer should know before tacking the pull request?