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

Refactor bioavailability handling #1130

Merged
merged 24 commits into from
Nov 30, 2023
Merged

Refactor bioavailability handling #1130

merged 24 commits into from
Nov 30, 2023

Conversation

kylebaron
Copy link
Collaborator

@kylebaron kylebaron commented Nov 14, 2023

Summary

This PR adds another member and methods to the datarecord class to track bioavailability (Fn).

A while ago, we had two different types of records: observations and events and mrgsolve employed some polymorphism to get these to work together.

a936b7e#diff-0e466cbf322abb6ca2747f70501b19721fced39bb696dfb1921ee40067bb5ece

In that setup, Fn was tracked in the event record object.

Since then, we collapsed this down into a single record type and tracking all data in the object (including dose amounts for observation records).

Part of that was to let Fn float around a bit, getting read from $MAIN when a record was getting processed and used in the various functions.

See #1129 for an example of when this doesn't work. So this PR is moving Fn back into the object so that this value will follow the object.

A bunch of tests were added too, including a key test for the problem that motivated this refactor.

Details

  • Fn is now a member of datarecord
  • Added fn() and fn(double) - getter and setter
  • Removed Fn that was getting passed into schedule() and steady(); this was never used
  • Data members were re-ordered to put members with same type next to each other; this might be more space efficient
  • One test in test-z-alag-f.R was specifically related to Incorrect F when F changes over the course of a lag time #1129; a bunch of other tests added to confirm behavior; a lot of this will also be confirmed in the NONMEM benchmarks

To consider

  • Because we had the polymorphism in the code previously, we were working with records as shared pointers. This isn't technically necessary now, but going to keep the code as is because (1) we might want to eventually go back to different record types and (2) there might be some advantage to using shared pointers for these records.

@@ -46,7 +46,7 @@ class datarecord {
int pos_, double id_);

//! short event constructor
datarecord(short int cmt_, int evid_, double amt_, double time_, double rate_);
datarecord(short int cmt_, int evid_, double amt_, double time_, double rate_, double fn_);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This constructor is used when copying data record objects; adding fn as a reminder that this is a key item that needs to make it into the new object.

@@ -76,7 +76,7 @@ class datarecord {
double rate(){return Rate;}
void rate(double value) {Rate = value;}

double dur(double b);
double dur();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Infusion duration; no longer needs Fn to be passed.

bool Output; ///< should this record be included in output?
bool Fromdata; ///< is this record from the original data set?
bool Lagged; ///< this record was added as result of ALAG
unsigned short int Ss; ///< record steady-state indicator
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These got re-arranged to get all the doubles and bools next to each other.

@@ -518,14 +517,16 @@ Rcpp::List DEVTRAN(const Rcpp::List parin,
if(this_rec->is_event()) {

this_cmtn = this_rec->cmtn();

if(!this_rec->is_lagged()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is key ... if this is a lagged dose (a phantom record) we cannot change bioavailability here.

@kylebaron kylebaron linked an issue Nov 15, 2023 that may be closed by this pull request
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.

I'm jumping ahead of the review request, but my understanding is that this is ready to go (with the already reviewed gh-1131 coming in on top). The background and motivation for moving bioavailability into the data record makes sense to me, and the code change and extensive tests look nicely done.

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.

Incorrect F when F changes over the course of a lag time
2 participants