Skip to content

Add weighted comparators - #577

Open
sumiya11 wants to merge 1 commit into
masterfrom
weights
Open

sumiya11 wants to merge 1 commit into
masterfrom
weights

Conversation

@sumiya11

Copy link
Copy Markdown
Collaborator

One design question is how to remember the indices of features when populating W.

Use example :

W = zeros(Int, 2, 15);
W[1, 12] = 1;             # First compare sparsity of Lie derivatives
W[2, 2] = W[2, 3] = 1;    # Then sums of degrees of num. and den.

using StructuralIdentifiability
import StructuralIdentifiability: cmp_weighted

# Goodwin
ode = @ODEmodel(
            x1'(t) = -b * x1(t) + 1 / (c + x4(t)),
            x2'(t) = alpha * x1(t) - beta * x2(t),
            x3'(t) = gama * x2(t) - delta * x3(t),
            x4'(t) = sigma * x4(t) * (gama * x2(t) - delta * x3(t)) / x3(t),
            y(t) = x1(t)
        );

res = reparametrize_global(ode, cmp=cmp_weighted(ode, W));

# hopefully something nice in res[1] ?..

@ChrisRackauckas

Copy link
Copy Markdown
Member

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

PR: #577

Reviewed head: 01f51a3
Raw SHA-256: 230a895d114a2319a2acc422272d750c26e9a46358d1f28f23b558081b9cf164
Source SHA-256: c60272d4735bb8ebbd659ab9df631e02d06335ab7ef9cdf82faed0944c245223
Source: standard-review-v1 (harness-log)
Reviewer session: 01a1d258-7599-7573-9b76-654146356967
Slot: 1
Confidence: unknown
Own risk fixed before author rating/discussion: yes

Risk assessment

  • Risk: medium-low (ML2)
  • Verdict: CHANGES
  • Reviewed head: 01f51a3

Review report

VERDICT: CHANGES
Round: 1
Head: 01f51a3
Slot: 1
CI: CI is not green
Session: 01a1d258-7599-7573-9b76-654146356967

Risk

medium-low (ML2: opt-in comparator ordering changes; existing default comparator calls are unchanged)

Blocking findings

  • Verdict: CHANGES
  • Round: 1
  • Blocking priorities: P2, P2
  1. [P2] Lie presets do not reproduce their named comparators — src/weights.jl:39. Confirmed by running: for @ODEmodel(x'(t)=z(t), z'(t)=x(t), y(t)=x(t)+a), let o=one(parent(ode)), f=x//o, and g=z//o. Each of cmp_lie(ode), cmp_lie_states(ode), and cmp_pushka(ode) returns true on (f,g), while cmp_weighted(ode, matrix_comparators[named]) returns false. The derivative features tie, and the weighted comparator falls back to comparing the original fractions. The named comparators compare the unequal derivatives instead. Across 576 pairs, the three presets disagree on 14, 12, and 6 pairs respectively. Preserve derivative-based tie handling for these presets and add pairwise regression tests.

  2. [P2] Accepted positive weights silently overflow — src/weights.jl:36. Confirmed by running: set W=zeros(Int,1,15) and W[1,3]=typemax(Int). On the same model, cmp_weighted(ode,W)(x//o,x^2//o) returns false. Only numerator degree has a nonzero weight, so a positive weight must rank degree 1 before degree 2. The unchecked Int score multiplication reverses that ordering through overflow. Compute scores without silent overflow and add a regression covering large accepted weights.

Non-blocking findings

  • src/weights.jl:21: a counting probe observed 16 Lie-derivative calls for one comparison, both with an all-zero matrix and with the default preset, whose Lie-feature weights are zero. Compute each required derivative once and skip inactive Lie features. This is a call-count observation; no wall-time benchmark was run.

What I ran

  • AI reviewer: codex-cli 0.164.0, model gpt-6.1-sol; actual session ID above, no conversation URL available.
  • git diff origin/master...HEAD, repository instructions, affected tests, and final git status --short --ignored: the checkout remains clean at the requested head. No PR discussion or earlier review was read.
  • Julia 1.13.2 in a scratch copy, preserving JULIA_DEPOT_PATH; Pkg.instantiate() resolved AbstractAlgebra 0.50.5, Nemo 0.56.2, RationalFunctionFields 0.3.6, and SciMLTesting 2.13.3.
  • GROUP=Core with Pkg.test(; julia_args=["--check-bounds=yes"], allow_reresolve=true): the full group passed on both completed runs. Affected suites passed: identifiable functions 800/800, Lie derivatives 6/6, observation field 1/1.
  • Julia probes with --check-bounds=yes: the initial weighted-comparator probe had 10 passing and 4 failing assertions. The corrected nonlinear/rational probe had 8 passing and 7 failing assertions across three additional models. Ordinary/state presets matched those corpora; Lie presets had the mismatches above. The public identifiable-function entry point completed with a weighted comparator. Separate instrumentation confirmed the 16 derivative calls.
  • uv run --locked python --version: Python 3.15.0; uv warned that --locked has no effect outside a project.

What I did not verify

  • No repository Julia manifest, Python project/uv.lock, or interpreter pin was available. Julia dependencies were resolved from Project.toml in scratch; the Python version command supplies no locked-environment validation.
  • QA, documentation builds, ModelingToolkit extension tests, other Julia versions/platforms, and wall-time performance. Remote CI details and author risk ratings were not inspected.

AUTHOR_GRADE: {"task":"feature","score":3,"difficulty":2,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"Core passes, but independent probes confirm Lie-preset mismatches and accepted-weight overflow. The PR adds no tests for the new functionality."}

Head link: 01f51a3


🤖 Posted by an AI agent — harness: codex-cli 0.164.0 · model: gpt-6.1-sol
Conversation: local session 01a1d258-7599-7573-9b76-654146356967 (no session URL)

This branch has not been deployed

No deployments
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.

2 participants