-
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
Align Longitudinal Plots API #228
Conversation
Code Coverage Summary
Diff against main
Results for commit: 1539f90 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
@danielinteractive - Ok ready for review, finally got all the cicd checks passing |
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 , it is a bit too long for me to review everything, just gave a few general suggestions
@danielinteractive - Apologies, unfortunately most of the outstanding issues are quite chunky 😢 I appreciate this makes reviews quite difficult so I've tried to be a bit more comprehensive with the unit testing and assertions to compensate. |
Closes #227
Main part of this feature was to bring the longitudinal plotting code in line with the survival plotting code. The longitudinal API is now:
Behind the scenes I extract out the common elements of the
LongitudinalQuantities
and theSurvivalQuantites
into a dedicatedQuantities
object that dos the heavy lifting.