Skip to content

petab1to2: rescale noise and priors when converting log10 to log - #535

Merged
FFroehlich merged 5 commits into
mainfrom
claude/issue-529-log10-noise-a37ec1
Oct 9, 2026
Merged

FFroehlich merged 5 commits into
mainfrom
claude/issue-529-log10-noise-a37ec1

Conversation

@FFroehlich

@FFroehlich FFroehlich commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

PEtab v2 has no log10-based noise distributions or priors, so petab1to2 has to express them on natural-log scale. Until now it did that without rescaling: log10-normal noise became log-normal with the same noise formula, which changes the likelihood, and an estimated sigma silently changed meaning by a factor of ln 10. log10-laplace noise raised NotImplementedError.

Why ln 10. ln(y) = ln(10) · log10(y). If log10(y) is normal (Laplace) with standard deviation (scale) σ, then ln(y) is normal (Laplace) with ln(10) · σ. The two densities are identical: the residual is (ln s − ln m) / (ln(10) σ) = (log10 s − log10 m) / σ, and the normalisation term gets the same ln 10 factor from the Jacobian. Priors work the same way: log10(x) ~ N(μ, σ) is ln(x) ~ N(ln(10) μ, ln(10) σ), and likewise for Laplace (location and scale).

Changes (petab/v2/petab1to2.py)

  • Observables: log10 + normal/laplace now map to log-normal/log-laplace, with the noise formula log(10) * (<formula>). The parentheses keep compound formulas intact, and noise placeholders are extracted as before. The old "Using log-normal instead" warning is replaced by a single warning that lists the affected observables. It points out that parameters in their noise formulas (e.g., an estimated sigma) keep their v1 meaning, i.e., they refer to log10(y); the ln 10 factor sits in the v2 noise formula.
  • Priors: parameterScaleNormal/parameterScaleLaplace on log10-scaled parameters now map to log-normal/log-laplace, with both prior parameters multiplied by ln 10. log10-laplace previously raised NotImplementedError. The prior warning is removed as well.

Tests (tests/v2/test_conversion.py)

  • Table level: log10-normal and log10-laplace map to log-*; the noise formula is log(10) * (...) and placeholders are unchanged.
  • End to end: v1 and v2 calculate_llh agree (rel. 1e-12) after petab1to2, for normal and laplace noise, with a numeric sigma and with noiseParameter1_obs_B + offset (an estimated sigma via the measurement table).
  • Priors: the v2 prior density matches the v1 Prior.pdf for log10/log/lin parameterScaleNormal/parameterScaleLaplace (rel. 1e-12, truncated at the bounds).
  • The new log10 tests fail on main and pass with this change. pytest tests/v2: 87 passed, 1 skipped.

Benchmark collection (checked locally, with a fixed simulation per observable and time): v1 and v2 log-likelihoods now agree to ≤ 4e-15 relative for all six affected models (Bachmann_MSB2011, Lucarelli_CellSystems2018, Schwen_PONE2014, Borghans_BiophysChem1997, Elowitz_Nature2000, Perelson_Science1996). The log10 parameterScaleNormal priors in Bachmann, Isensee, Raimundez and Schwen match the v1 densities to ~1e-13.

The issue's repro now prints the same value for v1 and for v2 as converted (2.667567).

Closes #529

🤖 Generated with Claude Code

PEtab v2 has no log10-based noise distributions or priors. Since
ln(y) = ln(10) * log10(y), a normal (Laplace) distribution of log10(y)
with standard deviation (scale) sigma is a normal (Laplace) distribution
of ln(y) with ln(10) * sigma.

* log10-normal and log10-laplace noise now become log-normal and
  log-laplace with the noise formula `log(10) * (<formula>)`.
  Previously, log10-normal became log-normal with an unchanged noise
  formula (changing the likelihood), and log10-laplace raised
  NotImplementedError.
* parameterScaleNormal and parameterScaleLaplace priors on log10-scaled
  parameters now become log-normal and log-laplace with both prior
  parameters multiplied by ln(10). Previously, log10-normal priors kept
  their parameters, and log10-laplace priors raised
  NotImplementedError.

Closes #529

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 8, 2026 12:37
@FFroehlich
FFroehlich requested a review from a team as a code owner October 8, 2026 12:37
@codecov-commenter

codecov-commenter commented Oct 8, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.69%. Comparing base (1a83733) to head (62fd87d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #535      +/-   ##
==========================================
+ Coverage   76.63%   76.69%   +0.06%     
==========================================
  Files          67       67              
  Lines        7527     7538      +11     
  Branches     1342     1344       +2     
==========================================
+ Hits         5768     5781      +13     
  Misses       1262     1262              
+ Partials      497      495       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The conversions are mathematically correct, preserve placeholder behavior, and have comprehensive regression coverage.

0 open findings

What changed in this PR

Corrects PEtab v1-to-v2 conversion so log10-based noise models and priors preserve their original probability densities.

Changes:

  • Rescales log10 normal/Laplace noise formulas and prior parameters by ln(10).
  • Adds table-level, likelihood, placeholder, and prior-density regression tests.
File Description
petab/​v2/​petab1to2.py Implements exact log10 noise and prior conversion.
tests/​v2/​test_conversion.py Verifies distributions, formulas, likelihoods, and prior densities.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

@dilpath dilpath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

In cases where the v1 noiseFormula is an estimated parameter sigma, then it's unclear whether this is an improvement, since then the v2 sigma should be interpreted with its log(10) factor too (i.e., a user of the converted v2 problem might get confused about whether the sigma is for the log10- or log- noise distribution).

A warning for this could be useful.

FFroehlich and others added 4 commits October 8, 2026 18:57
…0-noise-a37ec1

# Conflicts:
#	tests/v2/test_conversion.py
Parameters in the noise formulas of converted log10 observables (e.g.,
estimated noise standard deviations) keep their v1 meaning: they refer
to log10(y), with the ln(10) factor sitting in the v2 noise formula.
Warn about this once per conversion, listing the affected observables,
so users of the converted problem don't misread them as natural-log
noise parameters.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The merge of #532 removed the last use of `re`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@FFroehlich

Copy link
Copy Markdown
Collaborator Author

👍

In cases where the v1 noiseFormula is an estimated parameter sigma, then it's unclear whether this is an improvement, since then the v2 sigma should be interpreted with its log(10) factor too (i.e., a user of the converted v2 problem might get confused about whether the sigma is for the log10- or log- noise distribution).

A warning for this could be useful.

added warning

@FFroehlich
FFroehlich enabled auto-merge (squash) October 9, 2026 08:37
@FFroehlich
FFroehlich merged commit 37b7025 into main Oct 9, 2026
13 checks passed
@FFroehlich
FFroehlich deleted the claude/issue-529-log10-noise-a37ec1 branch October 9, 2026 08:38
FFroehlich added a commit that referenced this pull request Oct 9, 2026
…Uniform priors (#536)

Fixes three prior-conversion bugs in the PEtab v1 -> v2 converter
(`update_prior` in `v1v2_parameter_df`). They turned up while verifying
#535.

### Mappings

| v1 `objectivePriorType` | `parameterScale` | v1 parameters | before |
now |
|---|---|---|---|---|
| `logNormal` / `logLaplace` | any | `mu;sigma` | `logNormal` /
`logLaplace` verbatim → pydantic `ValidationError` when building
`v2.Parameter` | `log-normal` / `log-laplace`, `mu;sigma` |
| `parameterScaleUniform` | `log` | `a;b` (ln-space) | `log-uniform`
with `a;b` → prior pdf is NaN | `log-uniform` with `exp(a);exp(b)`,
clipped to the bounds |
| `parameterScaleUniform` | `log10` | `a;b` (log10-space) |
`NotImplementedError` (`log10-uniform`) | `log-uniform` with
`10^a;10^b`, clipped to the bounds |

Why these are correct:

1. **logNormal / logLaplace.** In v1 these are always based on the
natural logarithm, whatever the `parameterScale` (`Prior` maps
`(LOG_NORMAL, _)` to `Normal(..., log=True)`). That is the same
convention as v2 `log-normal` / `log-laplace`, so only the name changes.
2. **and 3. parameterScaleUniform on log/log10.** If ln(x) (or log10(x))
is uniform on [a, b], then x is log-uniform on [e^a, e^b] (or [10^a,
10^b]). v2 `log-uniform` takes the bounds of x itself as parameters.
`Parameter.prior_dist` notes this ("Mind the different interpretation of
distribution parameters for `Uniform(..., log=True)` and `LogUniform`").
So the parameters are transformed back to linear scale. Like v1's
`Prior`, the scaled interval [a, b] is first clipped to the scaled
parameter bounds, and the linear bounds are used for clipped endpoints.
Within the bounds this gives the same truncated density, and it avoids
overflow for valid but wide intervals such as log10 `-400;400`.

Missing `objectivePriorParameters` for `parameterScaleUniform` stay
missing, so `update_prior_pars` still fills in `lowerBound;upperBound`.
In v1 they default to the scaled bounds, which is the same distribution.

Not changed: a missing `objectivePriorType` (v1 default
`parameterScaleUniform`) still converts to `uniform` with
`lowerBound;upperBound` on any scale. On log-scaled parameters the
density differs from v1's log-uniform default. But this matches the v2
spec's default for parameters without a prior, and it keeps the optimum
of v1 problems that have no priors.

### Verification

For each case I compared `petab.v1.priors.Prior.from_par_dict(row,
type_="objective").pdf(x)` with the converted
`v2.Parameter.prior_dist.pdf(x)` on a grid spanning the bounds [1e-3,
1e2]. This included prior ranges inside the bounds, wider than the
bounds (truncation) and partly outside them. All agree to rel. 1e-12.

Tests: `test_petab1to2_parameter_scale_priors` (from #535) is now
`test_petab1to2_priors`. It is parametrized over prior parameters and
adds the cases above: log/log10 `parameterScaleUniform` within bounds,
exceeding bounds (including `-400;400` / `-1000;1000`, which overflow
when unscaled), and with missing parameters, plus
`logNormal`/`logLaplace` on `lin` and `log10` scale. 11 of the new cases
fail on the previous converter code, and all pass with this change.
`pytest tests/v2` passes (141 passed), including
`test_benchmark_collection` with `benchmark_models_petab` installed.
`pre-commit` (ruff, ruff-format) is clean.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

petab1to2: log10-normal noise becomes log-normal without rescaling the noise formula

4 participants