-
Notifications
You must be signed in to change notification settings - Fork 47
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
New function to plot trajectory of a model with custom timepoints. #739
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #739 +/- ##
============================================
+ Coverage 46.43% 89.65% +43.21%
============================================
Files 99 99
Lines 6671 6720 +49
============================================
+ Hits 3098 6025 +2927
+ Misses 3573 695 -2878
Continue to review full report at Codecov.
|
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.
Looks great 👍
I can make the suggested changes to set_custom_timepoints
within the next week, if you prefer.
Since this is especially useful for users, an example in a notebook would be appropriate.
timepoints = np.linspace(start=0, | ||
stop=end_time, | ||
num=n_timepoints) |
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.
Might be nice to have this handled by AmiciObjective.set_custom_timepoints
so it can be used in other contexts
stop=end_time, | ||
num=n_timepoints) | ||
obj = problem.objective.set_custom_timepoints( | ||
timepoints_global=timepoints) |
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.
Typehint says timepoints
can be a Sequence[np.ndarray]
, which would currently be specified with the timepoints
argument of set_custom_timepoints
instead. AmiciObjective.set_custom_timepoints
could be changed to have only one argument timepoints
that changes behaviour with isinstance
.
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.
👍
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, useful to have
# add timepoints as needed | ||
if timepoints is None: | ||
end_time = max(problem.objective.edatas[0].getTimepoints()) | ||
timepoints = np.linspace(start=0, |
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.
also use min(getTimepoints)
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.
no that would be a problem again, as in that scenario having only one measured timepoints at the end would give you no trajectory again 🤔 I would say in case you do not want to start at 0, just supply the timepoints.
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.
Ah, how are steady-state measurements handled?
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.
would you want a trajectory of a steady state? 🤔
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.
No but if there is an inf
timepoint then this might ruin the np.linspace
timepoints
return axes | ||
|
||
|
||
def _time_trajectory_model_with_states( |
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.
Could move this back and put plotStateTrajectories
inside an if state_indices:
block. The extra computation related to states should be insignificant
AMICI uses petab_problem.get_simulation_conditions_from_measurement_df() in |
ahh but in that case I would need to supply the petab_problem as well... will think about it a bit more and probably make an issue out of it and an extra PR. Better imo. |
… into added_more_options_to_model_fit
Thankful for any ideas on how to get the condition id's nicely. Otherwise, anything I forgot?