Make likelihoods a type of cost#230
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 210-add-likelihood-classes #230 +/- ##
==============================================================
- Coverage 94.37% 94.22% -0.15%
==============================================================
Files 33 33
Lines 1760 1750 -10
==============================================================
- Hits 1661 1649 -12
- Misses 99 101 +2 ☔ View full report in Codecov by Sentry. |
BradyPlanden
left a comment
There was a problem hiding this comment.
Looks great, thanks for the additions @NicolaCourtier! I've added a few comments, I'm happy to update these in #210 though :) Let me know if any thoughts on them!
| the log-likelihood | ||
| """ | ||
| raise NotImplementedError | ||
| self.log_likelihood = problem |
There was a problem hiding this comment.
This can be removed as it was only required for the ProbabilityCost() wrapper.
| self.log_likelihood = problem |
| assert likelihood._n_parameters == 1 | ||
| assert np.array_equal(likelihood._target, problem._target) | ||
|
|
||
| @pytest.mark.unit |
There was a problem hiding this comment.
I think we still want this test? Looking at the above class, BaseLikelihood still returns NotImplemented from call(). It would be good to keep a test on that.
| ): | ||
| self.cost = cost | ||
| self.x0 = x0 | ||
| self.x0 = cost.x0 |
There was a problem hiding this comment.
As mentioned above. This keeps it open :)
| self.x0 = cost.x0 | |
| self.x0 = x0 or cost.x0 |
| @@ -40,27 +40,24 @@ class Optimisation: | |||
| def __init__( | |||
| self, | |||
| cost, | |||
There was a problem hiding this comment.
I think we want users to overwrite the initial conditions, right? This modification should keep that open.
| cost, | |
| cost, | |
| x0=None, |
Description
Suggestion to make the new likelihood classes a type of cost.
Issue reference
Regarding #210
Type of change