petab1to2: order placeholders by index, not as text

[FFroehlich]

petab1to2 sorted an observable's placeholders as text when building the v2 observablePlaceholders / noisePlaceholders columns. The v1 overrides in the measurement table are positional, so from 10 placeholders on (observableParameter10_x sorts before observableParameter2_x), the override values were silently assigned to the wrong placeholders.

Changes:

• Take the observable and noise placeholders from v1.get_formula_placeholders, the helper the v1 linter uses. It orders them by the index in their ID.
• If an observable's placeholders are not numbered consecutively from 1 (e.g. only observableParameter1_x and observableParameter3_x), raise a ValueError. The positional overrides can't be mapped in that case. In petab1to2, v1 linting already rejects such problems ("Non-consecutive numbering of placeholder parameter"). The new check covers direct calls to v1v2_observable_df.

Tests:

• test_petab1to2_placeholder_order: converts a problem with 12 observable placeholders and 11 noise placeholders, and checks that each placeholder gets its own override value.
• test_v1v2_observable_df_placeholder_gap: checks that a skipped index raises the error, for observable and noise placeholders.

The repro from the issue now prints observableParameter<i>_obs_B = i for all 11 placeholders.

Closes #531

🤖 Generated with Claude Code

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.63%. Comparing base (921ce8e) to head (e660eb1).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #532      +/-   ##
==========================================
- Coverage   76.63%   76.63%   -0.01%     
==========================================
  Files          67       67              
  Lines        7529     7527       -2     
  Branches     1342     1342              
==========================================
- Hits         5770     5768       -2     
  Misses       1262     1262              
  Partials      497      497              

☔ 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]

🟢 Approval recommended

The localized fix preserves positional mapping, with focused regression coverage and no unresolved findings.

0 open findings

What changed in this PR

Fixes #531 by preserving positional override mapping during PEtab v1-to-v2 conversion.

Changes:

• Sorts observable and noise placeholders by numeric index.
• Rejects placeholder numbering that is not consecutive from 1.
• Adds regression tests for 10+ placeholders and numbering gaps.

File	Description
tests/​v2/​test_conversion.py	Tests override alignment and gap rejection for both placeholder types.
petab/​v2/​petab1to2.py	Sorts placeholders numerically and validates consecutive numbering.

🧠 Review effort: Balanced

---

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

[dilpath]

👍

[dilpath]

Instead of re-implementing here, can the logic from the linter be called here? Fine as is, if it's not straightforward.

Also ideally the same check isn't applied to a problem twice, which this PR might introduce based on

In petab1to2, v1 linting already rejects such problems ("Non-consecutive numbering of placeholder parameter"). The new check covers direct calls to v1v2_observable_df.

So we could move all checks from petab1to2 to the v1v2_* methods? Or if this is the only v1v2_ method where it is relevant to recheck, then fine.

[FFroehlich]

Good point, done in bfaf9e4: extract_placeholders now calls v1.get_formula_placeholders, the helper the v1 linter uses. It returns the placeholders ordered by index and raises if they aren't numbered 1..n; that error is re-raised as a ValueError naming the observable. As a side effect, placeholders are now found the same way as in v1's override-count check (by text match on the formula rather than sympy's free symbols, which drop a placeholder that cancels out).

On applying the check twice: there is no separate check anymore. The conversion needs the index-ordered list anyway, and the error comes with it. I'd still keep it in v1v2_observable_df rather than rely on the linter alone, because #534 adds validate=False, which skips v1 linting entirely. Without this, that path would silently produce a misaligned placeholder list. I wouldn't move the other lint checks into the v1v2_* functions, since many of them compare tables against each other (overrides vs. observables, parameter table vs. model), which a single-table function can't see.

[dilpath]

Thanks.