separate HL reason checking from filter_slopes#929
Conversation
Gero1999
changed the title
| #' @examples | ||
| #' # Suppose processed_pknca_data is a valid PKNCA data object | ||
| #' # check_valid_pknca_data(processed_pknca_data) | ||
| check_valid_pknca_data <- function(processed_pknca_data, exclusions_have_reasons = TRUE) { |
There was a problem hiding this comment.
Since you have separated the logic from filter slopes, is there a reason to have the parameter exclusions_have_reasons ? Since the whole purpose of this function is to check validity then its implied that we are checking if the exclusions have reasons, and there is no use for the function otherwise...
If you have a reason for keeping it, then it should be renamed as exclusions_have_reasons doesn't quite make sense. I would suggest check_reasons or something similar
There was a problem hiding this comment.
good question. I want to let us further include more optional checking args here and also at some point probably allow the user if they want them or not to be checked. But I like the renaming to check_exclusion_has_reason
use incoming slope_rules()$... instead of slope_rules$...() without the check in filter_slopes() Merge remote-tracking branch 'origin/main' into 928-enhancement-separate-hl-reason-checking-from-filter_slopes # Conflicts: # inst/shiny/modules/tab_nca.R
js3110
left a comment
js3110
left a comment
There was a problem hiding this comment.
I think this warning message isn't super clear for the user:
maybe we remove the row info all together (since they can just check which exclusions are missing a reason), or incorporate a paste?
|
@js3110 maybe better to open a new issue to customize the message (good-first issue) and don't overexpand this PR |
Hmm I woudl disagree seeing as this PR directly changes the message, so its relevant and in-scope. It wouldn't add more to this PR just change what has already been done |
|
I moved the code and refactor but the logic display should be the same. But if you are keen I can simply take off the information on the exclusion missing, that is fine for me |
The display is different, here is the example from main: clearly there is something wrong with this version as there are two sentences telling the user to go to slope selector to add the reason, and the pasting of the data is different. So I would suggest you change it back to how it was before, or remove the data. Whatever you prefer |
|
Not sure what happened, the row should have been already removed, did you pull? @js3110 |
|
It works for me: |
| if (any(missing_reasons)) { | ||
| stop( | ||
| "No reason provided for all half-life exclusions.\n", | ||
| "Please go to `Setup > Slope Selector` and type a REASON in the table for each." |
There was a problem hiding this comment.
well more like for each half-life exclusion (rows can also be half-life selections). Do you think is better be explicit with the repetition?
yes, works now. I would still change phrasing slightly as it currently says |
Gero1999
deleted the
928-enhancement-separate-hl-reason-checking-from-filter_slopes
branch
3 participants
Issue
Closes #928
Description
check_valid_pknca_datato check that all manually excluded half-life points have a non-empty reason; if any are missing, an error is raised and the affected rows are printed. This function is now called before NCA calculations, and can be used to keep expanding the checks.check_reasonsparameter and related logic from thefilter_slopesfunction, delegating responsibility for exclusion reason validation to the newcheck_valid_pknca_datafunctioncheck_valid_pknca_datato verify correct behavior when exclusions have reasons and when they do not.Definition of Done
How to test
Map data > NCA > Slope Selector > Double click (add exclusion) > Try run NCA > See message > Populate REASON in Slope Selector > Run NCA
Contributor checklist