-
Notifications
You must be signed in to change notification settings - Fork 13
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
add t_inf to sc doses in new_regimen #97
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a helpful extension to the code base, since SC regimens should have t_inf flexibility.
Based on reading the code, I think the intention of the code is not quite what is currently implemented. I have added a couple tests that do not currently pass, but that I think are supposed to pass. Can you please update the logic so that they pass, or if they should not pass, we should make that clearer to the end user.
In particular, I think the NA checks on line 73 and 78 are at odds with each other since they will result in different imputations if both SC and infusion are in the regimen, versus just one or the other.
@jasmineirx Thank you! That makes a lot of sense. I have updated the logic to assign the correct t_inf default per type. I also included im alongside sc as a default of 1/60, given the similarity of these routes. I also updated the tests you had added to be slightly broader, but still testing the same scenarios. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a suggestion for reducing the repetitiveness of the code while balancing readability
Co-authored-by: Jasmine Hughes <43552465+jasmineirx@users.noreply.github.com>
Co-authored-by: Jasmine Hughes <43552465+jasmineirx@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Added t_inf argument to new_regimen() and adapted tests.