Wragged output methods #1405
Wragged output methods #1405
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1405 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 87 87
Lines 8889 9018 +129
==========================================
+ Hits 8889 9018 +129
Continue to review full report at Codecov.
|
|
@MichaelClerx would you be able to take a look here if you get a moment? |
|
I'll try to look before too long. Are there no PKPD modellers who could have a look in the meantime? |
|
Thanks! @martinjrobins -- if you had a spare few mins, it would be good to see what you thought about this. |
There was a problem hiding this comment.
Hi Ben,
It looks quite good, but I'm a bit confused that the culmination of all of this is a custom-made likelihood class. I've thought about it a bit, and can't see any easy way out other than adding a Composed/Combined/Composite Likelihood, Prior, and Error in the same PR?
We've got a ComposedLogPrior already: https://github.com/pints-team/pints/blob/master/pints/_log_priors.py#L170
| Pints provides the :class:`SingleOutputProblem` and | ||
| :class:`MultiOutputProblem` classes to formulate | ||
| Pints provides the :class:`SingleOutputProblem`, | ||
| :class:`MultiOutputProblem`, :class:`ProblemCollection` |
There was a problem hiding this comment.
Not a blocker: But can we think of some names that make it clear that:
- A ProblemCollection can't just contain any old problem. We can't add singleoutput problems, for example
- The ProblemCollection and SubProblem go together
I'm not sure what these names would be :D
MultipartProblem and ProblemPart ?
There was a problem hiding this comment.
Maybe get the word "output" into the name for ProblemCollection, to show that all 3 problem types are actually classified by their output. So SingleOutput, MultiOutput, and... SplitOutputProblem? ComposedOutputProblem?
There was a problem hiding this comment.
Since the user never creates a SubProblem, (and since it is a very different thing from the other 3 classes called XProblem), is this a rare case for having a class inside a class?
E.g.
class ProblemCollection(object):
...
class SubProblem(object):
...
def subproblem(i):
return ProblemCollection.SubProblem(...)
| @@ -99,6 +99,7 @@ relevant code. | |||
| - [Multiplicative Gaussian noise](./stats/multiplicative-gaussian-errors.ipynb) | |||
| - [Pooling parameters](./stats/pooling.ipynb) | |||
| - [Student-t noise model](./stats/student-t-sampling-error.ipynb) | |||
| - [Wragged output model](./stats/multiple_wragged_outputs.ipynb) | |||
There was a problem hiding this comment.
I've never come across the word "wragged" before. Is it a technical term?
| "source": [ | ||
| "# Multi-output model with wragged time measurements\n", | ||
| "\n", | ||
| "This notebook describes how to fit a model in PINTS which has multiple outputs with different measurement times for each. Additionally, we illustrate how this same framework allows different measurement models to be used for different outputs within the same forward model when doing inference.\n", |
There was a problem hiding this comment.
This is too quick for me.
Can we change it to say "This notebook describes... for each. This situation arises when..." 1-3 lines on why this would occur, e.g. when you measure different system outputs individually.
Then leave the "additionally" for later, just add in a header and give a 2nd intro?
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "Now we assume that only the first half of time measurements are taken for the first two outputs; and only the second half for the third." |
There was a problem hiding this comment.
Can we make this a bit less artificial? I.e. start with wordy version, saying e.g. we measure X every 6 hours, but Y only once a day. Then show how that leads to code like given below?
| "# solve system\n", | ||
| "sol_12 = problem_0.evaluate(real_parameters)\n", | ||
| "\n", | ||
| "## this uses cached result from first solve under the hood\n", |
There was a problem hiding this comment.
| "## this uses cached result from first solve under the hood\n", | |
| "# this uses cached result from first solve under the hood\n", |
| }, | ||
| "outputs": [], | ||
| "source": [ | ||
| "class CombinedLogLikelihood(pints.LogPDF):\n", |
There was a problem hiding this comment.
This is exactly the kind of thing I'd want PINTS to handle for me, after I've gone to the trouble of setting up the special problem class!
Implements
SubProblemandProblemCollectionmethods which collectively allow: