Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix until calculation when time not equal to 0 #1154

Merged
merged 8 commits into from
Jan 22, 2024
Merged

Fix until calculation when time not equal to 0 #1154

merged 8 commits into from
Jan 22, 2024

Conversation

kylebaron
Copy link
Collaborator

@kylebaron kylebaron commented Jan 17, 2024

@dpastoor reported bad behavior of until when constructing event objects #1144

The primary problem was that the logic inadvertently assumed the dose was given at time = 0. This PR fixes that logic.

One legit question raised in the issue was when should dosing end when this works properly? For example, Q28d dosing that runs until 168` hours. The dosing regimen ends at 168 hours; should there be a dose given at 168 hours or not?

As I was working on this issue, I was thinking there should be a dose there. But it looks like up to this point, we have been assuming that there isn't a dose at that time. So I'm going to code this fix to respect that behavior even though I'm thinking about it a little different now.

To be clear: the until argument is the expected last observation time covered by that dose sequence; so we keep you dosing to cover the until time. If a dose might be given at the same time as the last observation, until won't cover that.

Using @dpastoor 's example, the dosing runs to 84 hours, not 112. But I agree it might be reasonable to run it to 112 too.

library(mrgsolve)
#> 
#> Attaching package: 'mrgsolve'
#> The following object is masked from 'package:stats':
#> 
#>     filter

realize_addl(ev(amt = 300, time = 4*7, ii = 4*7, until = 16*7-0.001))
#> Events:
#>   time amt ii addl cmt evid
#> 1   28 300  0    0   1    1
#> 2   56 300  0    0   1    1
#> 3   84 300  0    0   1    1
realize_addl(ev(amt = 300, time = 4*7, ii = 4*7, until = 16*7))
#> Events:
#>   time amt ii addl cmt evid
#> 1   28 300  0    0   1    1
#> 2   56 300  0    0   1    1
#> 3   84 300  0    0   1    1
realize_addl(ev(amt = 300, time = 4*7, ii = 4*7, until = 16*7+0.001))
#> Events:
#>   time amt ii addl cmt evid
#> 1   28 300  0    0   1    1
#> 2   56 300  0    0   1    1
#> 3   84 300  0    0   1    1
#> 4  112 300  0    0   1    1

Created on 2024-01-16 with reprex v2.0.2

EDIT: this PR also modernizes the ev() help topic and adds some text and examples for what to expect for until behavior. There are some other modifications that I'd like to make, but this will require modifying some other help topics as well (e.g. documenting evid/EVID in data_set). I will do this as another PR.

@kylebaron kylebaron requested a review from kyleam January 17, 2024 05:32
@dpastoor
Copy link
Contributor

thanks kyle! I can also test kicking the tires on this if you'd like

@kylebaron
Copy link
Collaborator Author

Thanks, @dpastoor ; that'd be great if you could.

tests/testthat/test-ev.R Outdated Show resolved Hide resolved
@kylebaron kylebaron requested a review from kyleam January 20, 2024 14:17
Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. LGTM

As I was working on this issue, I was thinking there should be a dose there. But it looks like up to this point, we have been assuming that there isn't a dose at that time. So I'm going to code this fix to respect that behavior even though I'm thinking about it a little different now.

I too would expect a dose there, but I agree with respecting the current behavior.

Copy link
Contributor

@dpastoor dpastoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewise LGTM - thank you!

thinking very much out loud and I am on the fence if this would even be that helpful....

one thought that this idea brings up, is a potential utility function through - at first I was thinking a separate param through, but that would clutter the api.

Thinking about a reference example of use of fixed() for use in stringr such that you can do like str_detect(strings, pattern) or str_detect(strings, fixed(pattern))

where fixed makes the comparison at the byte literal level instead of a regex.

ev(amt = 100, until = through(16*7)) where that would literally just be

through <- function(.x) {
.x + 0.001
}

This would also setup through to be adaptable or have some heuristic based on time scales? Is that even a thing?? where in some situations incrementing by 0.0001 vs 0.01 vs 0.00000001 may actually meaningfully impact a choice?

half of the value would be calling out that officially in usage where people may be more likely to read it - eg

image

is very clear vs having to find it in a nested example.

#' @param ID subject ID.
#' @param replicate logical; if `TRUE`, events will be replicated for
#' each individual in `ID`.
#' @param until the expected maximum **observation** time for this regimen;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks this is much clearer!

@kylebaron
Copy link
Collaborator Author

Thanks for sketching that out, @dpastoor. This is definitely a "thing" in mrgsolve ... because we implement doses and ask for observations at well-ordered intervals, we frequently need to "decide" what to do when when these happen at the same time. Will keep thinking about this and see if any patterns emerge that are easy to grasp and clarify code.

I think we're all on the same page that retaining current behavior is the right thing to do at this point. For future consideration, I do think this design decision is the right thing to do and what we want most of the time. Even though the until functionality seem like it should produce a dose at the until time, I see that time as the time that we want to either end the simulation or we want to start some other dosing regimen. For this example

> ev(amt = 100, ii = 4*7, until = 16*7) %>% realize_addl()
Events:
  time amt ii addl cmt evid
1    0 100  0    0   1    1
2   28 100  0    0   1    1
3   56 100  0    0   1    1
4   84 100  0    0   1    1
> 16*7
[1] 112

We'd want to give the 100 mg dose until 16 weeks and at that point, start a new dose. In that case, we'd want to save 16*7 for the new dose. Maybe other ways to see it but I think this is the motivation behind the original decision.

library(mrgsolve)
#> 
#> Attaching package: 'mrgsolve'
#> The following object is masked from 'package:stats':
#> 
#>     filter
e1 <- ev(amt = 100, ii = 4*7, until = 16*7)
e2 <- ev(amt = 200, ii = 4*7, until =  32*7)
ev_seq(e1, e1)
#> Events:
#>   time amt ii addl cmt evid
#> 1    0 100 28    3   1    1
#> 2  112 100 28    3   1    1

Created on 2024-01-21 with reprex v2.0.2

@kylebaron kylebaron merged commit 5b21e18 into main Jan 22, 2024
@kylebaron kylebaron deleted the fix/until branch January 22, 2024 00:31
@kylebaron kylebaron mentioned this pull request Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants