Add v2.calculate / fix v1.calculate#395
Conversation
Mostly a copy of `v1.calculate` with a few adaptations to handle the new noise distributions and placeholder format. Without a significant rewrite, it's difficult to reduce redundancy. I'd leave that as is for now until this gets updated to use the new object model once finalized.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #395 +/- ##
===========================================
+ Coverage 73.81% 73.96% +0.14%
===========================================
Files 58 60 +2
Lines 6493 6637 +144
Branches 1120 1152 +32
===========================================
+ Hits 4793 4909 +116
- Misses 1277 1289 +12
- Partials 423 439 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
petab/v2/calculate.py
Outdated
| (simulation_df[col] == row[col]) | is_empty(row[col]) | ||
| for col in compared_cols | ||
| ] | ||
| mask = reduce(lambda x, y: x & y, masks) |
There was a problem hiding this comment.
Or mask = reduce(operator.and_, masks) but fine
tests/v2/test_calculate.py
Outdated
| simulation_df[SIMULATION] = [2, 3] | ||
|
|
||
| expected_residuals = { | ||
| (np.log(2) - np.log(0.5)) / (2 * 7 + 8 + 4 + np.log(2)), |
There was a problem hiding this comment.
Not sure, should the np.log(2) be just 2 in the denominator? Is there a reference for this? I find it unusual to choose lognormal noise but then also include the observable in the noise formula -- would be good to define how this should be handled. The \sigma in the lognormal formula doesn't have a log transformation, so I would say it's just 2 in the denominator...
There was a problem hiding this comment.
Hm, good catch! For PEtab v2 you are right.
For v1, I am not sure. For observableTransformation it says "Transformation of the observable and measurement for computing the objective function.". Since it doesn't specify where in the objective function, I'd interpret it as everywhere. However, I am not sure if that was the original intention...
There was a problem hiding this comment.
I think the only reason to have this observableTransformation=log was to support lognormal distributions (which is also how I've seen Jan refer to it). So, I think the description should have been this explicitly, rather than the current docs. But I don't understand why observableTransformation was chosen as the column name, so I am also unsure...
There was a problem hiding this comment.
Agreed. Considering https://petab.readthedocs.io/en/latest/v1/documentation_data_format.html#noise-distributions, I think it's valid to conclude that the description of observableTransformation is incorrect.
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
3 participants
Mostly a copy of
v1.calculatewith a few adaptations to handle the new noise distributions and placeholder format.Without a significant rewrite, it's difficult to reduce redundancy. I'd leave that as is for now until this gets updated to use the new object model once finalized.