Bug: AUC start 0 (which also needs Feature: C0 imputation)

[Gero1999]

Issue

Closes #2 #60

Description

After running NCA, AUC calculations always consider the discrete start (start at where first concentration sample was collected). This can indeed alter slightly the real values of the AUC. This is a consequence of 2 problems:

1. Doses times are being considered at the first measure, instead of at the real dose time
2. No imputed measure at dose time is consdiered

Change description.

Definition of Done

C0 imputation option for the user specified in Setup > Data Selection

How to test

• At "NCA">"NCA Results", when C0 is imputed it is expected that start=AFRLT-ARRLT (being AFRLT-ARRLT= 0 in DOSNO=1)
• At "NCA">"NCA Results", when C0 is not imputed it is expected that start={time of 1st observation}
• AUC values (auclast, aucinf...) from start=AFRLT-ARRLT are calculated when C0 is imputed
• All variables that should only be based on observed data (cmax, tmax...) are not considering C0 imputations
• Test that PKNCA also deals well with predoses as duplicates for the NCA
• Check slope selector plots and NCA results remain well after running NCA
• Check if in general C0 imputation logic works well (NCA experts):
  if C0 exists ~ C0=C0
  if DOSNO=1 & IV bolus (ADOSEDUR=0, ROUTE=intravascular) ~ C0=0
  if DOSNO>1 & not IV bolus ~ C0=predose
  if IV bolus and monoexponential data ~ logslope
  if IV bolus and not monoexponential data ~ C0=C1

Contributor checklist

• 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?

[m-kolomanski]

Hey,

Are the diffs displayed by github accurate? Seems like a lengthy change, are there any artifacts/errors that happened due to merge (it did happen to me on occasions)? If the diffs are inaccurate, please point out the actual changes and disregard the below.

I do not have the time to do a detailed review (it is quite a lengthy change), but after a quick look there is some general things I want to point out:

1. Please do not any files that solely generate UI to the /R folder - this is a folder for our logic. File partial_auc_input belongs in the inst/shiny folder, either in /modules (if there is any server logic attached, here it isnt, but might be added) or folder like functions. We can also create a new folder for UI elements, called ui or elements or widgets or whatever you find suitable.
2. If you are creating new files or adding new functions in the /R folder, please make sure that this logic is testable and do take some effort to write at least basic unit tests. Adding tests back to existing logic is tough and time consuming, so we should at least make sure that our testing suite keeps up with any new features or major changes to the old ones.

[Gero1999]

Hey,

Are the diffs displayed by github accurate? Seems like a lengthy change, are there any artifacts/errors that happened due to merge (it did happen to me on occasions)? If the diffs are inaccurate, please point out the actual changes and disregard the below.

I do not have the time to do a detailed review (it is quite a lengthy change), but after a quick look there is some general things I want to point out:

1. Please do not any files that solely generate UI to the /R folder - this is a folder for our logic. File partial_auc_input belongs in the inst/shiny folder, either in /modules (if there is any server logic attached, here it isnt, but might be added) or folder like functions. We can also create a new folder for UI elements, called ui or elements or widgets or whatever you find suitable.
2. If you are creating new files or adding new functions in the /R folder, please make sure that this logic is testable and do take some effort to write at least basic unit tests. Adding tests back to existing logic is tough and time consuming, so we should at least make sure that our testing suite keeps up with any new features or major changes to the old ones.

Hello and happy new year @m-kolomanski !

You are right, this is a very lengthy change but I re checked and everything was intentional. Most of them are new created manuals for each function. As requested I also:

1. Deleted the extra UI function from \R
2. Added tests for all functions called in format_data,R, create_start_impute and PKNCA_impute_method_additions

Don't hesitate to let me know if there is anything else I can do or if there are other concerns that I can address! I also asked @js3110 for re-review to make sure all looks as expected.

[js3110]

LGTM

[m-kolomanski]

Hey, happy new year to you too and thank you for all the hard work!

I have to request some changes and have some suggestions/nitpicks to consider. I know that some of my comments refer to lines that were not changed in this PR, but since we are working on those functions I think it would be beneficial to go back and improve the code, even if it is not directly related to the changes.

[m-kolomanski]

question: What is ret in this context? Might I suggest a more descriptive name for the variable?

[Gero1999]

I asked Bill because I actually followed his syntax from the other parallel functions. Apparently is from the word return (of the function). But I will change it to something more intuitive now

[Gero1999]

I put d_conc_time, hope that makes more sense!