-
Notifications
You must be signed in to change notification settings - Fork 4
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
Support Negative Time / Re-work Simulation function #282
Conversation
@danielinteractive - This is now ready for review. Apologies this ended up being much larger than expected, I appreciate this is more than what should be in a single PR so let me know if you'd prefer to book some time to go over it together. Main thing is that there are some small updates to the Stan code to ensure both the GSF and SF models work with negative times and then a major overhaul of the simulation functions to bring their syntax inline with what is used for the core model specification. |
Unit Tests Summary 1 files 40 suites 7m 50s ⏱️ Results for commit 8e761c3. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit a8d864f ♻️ This comment has been updated with latest results. |
Code Coverage Summary
Diff against main
Results for commit: 8e761c3 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
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.
Thanks @gowerc , please see few comments inside, if you have something specific you would like me to have a careful look at please let me know :) (a bit too large for a detailed review indeed :))
@danielinteractive - Tbh I'm not that worried about the simulation code, at this point it is very well tested and is more auxiliary anyway. If you could focus your attention just on the Stan code changes I think that is more the critical part. In particular just double checking that indeed the changes mean that 0 and negative times are correctly supported. Talking to Francois today he mentioned about generating quantities for negative times which is something I haven't yet tested so will add some more tests for that. |
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.
Thanks @gowerc , stan code changes look good to me. It is also important to have a few tests regarding negative and zero values in place, but probably you already have those I guess.
Closes #215
This is still a WIP
This ended up being more complicated than expected. Well its just that the current simulation setup could support negative times it was just very clunky and non-intuitive so I took this as an opportunity to overhaul the simulation functions and bring them to parity with the regular JointModel functions (e.g. the api is a lot closer now with key terms being the same with simply a
Sim
prefix).Am still updating the test files at the moment as unfortunately the simulation functions are used in a lot of places. Will provide a more broader explanation once I've finished coding.