Enhancement: Mean plots CV% #103
Conversation
m-kolomanski
requested review from
m-kolomanski
and removed request for
m-kolomanski
Enhancement: allow to color by multiple variables individual plots
…of github.com:pharmaverse/aNCA into Enhancement-implement-CI-workflows-based-on-admiralci
There was a problem hiding this comment.
Hey, good job in general, code works and is quite clean, thanks for including fine-grained the tests, however I have few comments:
issue(blocking): Code does not pass devtools::lint() check.
question, suggestion: For datapoints close to zero, CV is so large that the line is completely horizontal - is that expected and correct behaviour? Migth be worth setting some limits to keep the plot readable?
suggestion: When both SD and CV are selected, they will overlap each other. Might be worth thinking about different approach to display them, or allow only one at the time (in the UI, if one is selected, the other is automatically deselected).
Co-authored-by: Mateusz Kołomański <[email protected]>
There was a problem hiding this comment.
Hey, looks fine, I have just a minor issue regarding the naming of variables.
I can see that spellcheck and lintr CI are failing, but looking at the errors, I think it is due to lacking WORDLIST and some rules from .lintr on this branch. Rebasing to main or copying those files over should solve the issue, but might create some conflicts that might have to be solved before merging.
R/general_meanplot.R
Outdated
| geom_line(aes(colour = id_variable)) + | ||
| geom_point(aes(colour = id_variable)) + | ||
| facet_wrap(~groups, | ||
| strip.position = "top") + | ||
| labs( | ||
| x = paste0("Nominal Time [", preprocessed_data$RRLTU[1], "]"), | ||
| x = paste0("Nominal Time [", timeunit, "]"), |
There was a problem hiding this comment.
| x = paste0("Nominal Time [", timeunit, "]"), | |
| x = paste0("Nominal Time [", time_unit, "]"), |
R/general_meanplot.R
Outdated
| y = paste0( | ||
| "Mean concentration", " [", paste(unique(preprocessed_data$AVALU), collapse = ","), "]" | ||
| "Mean concentration", " [", concunit, "]" |
There was a problem hiding this comment.
| "Mean concentration", " [", concunit, "]" | |
| "Mean concentration", " [", conc_unit, "]" |
Co-authored-by: Mateusz Kołomański <[email protected]>
@m-kolomanski i tried to rebase but it doesn't seem to be working as it doesn't show me any conflicts it just changes the file to the main version. Are we not able to merge anyway despite the spellcheck fail? |
Co-authored-by: Mateusz Kołomański <[email protected]>
Co-authored-by: Mateusz Kołomański <[email protected]>
Co-authored-by: Mateusz Kołomański <[email protected]>
…CA into enhancement/mean-plots-cv
|
@js3110 I have rebased the branch, I believe all checks should pass, when they do you are free to merge |
Issue
Closes #95
Description
Mean plots additional option to show CV% as well as standard deviation
Definition of Done
Mean plots show CV% when selectedHow to test
Outputs -> Mean plots -> Show CV%
Contributor checklist