Macros to determine the last record for an individual or a simulation#1327
Macros to determine the last record for an individual or a simulation#1327
Conversation
| prob.rown(crow); | ||
| } | ||
|
|
||
| this_rec->id(id); |
There was a problem hiding this comment.
Moved this up for clarity.
| double maxtime = a[i].back()->time(); | ||
|
|
||
| const double maxtime = a[i].back()->time(); | ||
| const int NNI = dat.inrow(i); |
There was a problem hiding this comment.
The total row count for "this" individual.
| ans(crow,1) = this_rec->time(); | ||
| ans(crow,0) = id; | ||
| ans(crow,1) = tto; | ||
| for(unsigned int k=0; k < n_capture; ++k) { |
There was a problem hiding this comment.
Make consistent with main output writing code.
kyleam
left a comment
kyleam
left a comment
There was a problem hiding this comment.
Two non-blocking comments, but looks good to me.
| this_rec->id(id); | ||
| tto = this_rec->time(); | ||
|
|
||
| if(icrow==NNI || crow==NN || tto > maxtime) { |
There was a problem hiding this comment.
Is it correct that this new tto > maxtime condition isn't an essential change, and it just avoids the downstream ->schedule call being a no-op? I removed the condition and the tests still pass.
There was a problem hiding this comment.
Yeah; it's probably on the conservative side. I discovered that it was possible for some in consequential events happening after we hit the last official output record for a given individual. For example, we only calculate the end time of an infusion at the time when it starts, not when the dose is scheduled to happen (potentially in the future through addl) and I realized that we are allowing infusions to end after the last output record and the system is advancing to that time. I'm not totally sure what the right thing to do there. I'm guessing we could just decline to schedule the "infusion off" event if it would happen after maxtime. But I didn't want to get into that in this PR.
But you're right ... if maxtime (specific for an individual) is tied to the time when icrow==NNI, then we probably don't event need crow==NN, right? the NN test and the NNI test should both be true on the last record of the data set. I originally had this continue clause in there because crow references a row in the output matrix and you'd get bad behavior in case that count was ever off, so it's sort of fear-based decision :).
Could you test with just icrow==NNI?
if(icrow==NNI || crow==NN || tto > maxtime) {
continue;
}There was a problem hiding this comment.
Also ... I did open an issue to at least review cases where we can be more careful of turning stuff off.
#1328
There was a problem hiding this comment.
Thanks for the details/explanation. Leaning conservative/safe is of course fine by me.
Could you test with just
icrow==NNI?
Yes, the tests still all pass on my end with the condition reduced to if(icrow==NNI).
There was a problem hiding this comment.
Thanks for checking that. I was almost going to push a commit that removed tto > maxtime, but think I'm going to leave it for now. I still need to fix the issue in #1324 and I think once that is behaving, I can write some check for correctness of the counts. At that point, will go back and see if we can simplify these checks, which I do think we should be able to do.
2 participants
FINAL_ROWsignals we just advanced to the final row of the output dataFINAL_IROWsignals we just advanced to the final row of the current individualBoth have been added to the RESERVED word list.
See #1328