Skip to content

Count snapshot rows from scalar unknowns so array-valued unknowns reduce - #218

Open
singhharsh1708 wants to merge 2 commits into
SciML:mainfrom
singhharsh1708:hs/array-valued-unknowns
Open

singhharsh1708 wants to merge 2 commits into
SciML:mainfrom
singhharsh1708:hs/array-valued-unknowns

Conversation

@singhharsh1708

Copy link
Copy Markdown
Member

Fixes #216. MethodOfLines 1.6 keeps each discretized dependent variable as one array-valued unknown, so length(unknowns(sys)) no longer counts solution rows and deim/pod rejected its problems with DimensionMismatch (two unknowns against twelve rows in test/deim.jl). The row counts in deim, pod and FullOrderModel now come from scalar_unknowns(sys), which expands array-valued unknowns into their elements in column-major order, the order the solution rows use; scalar unknowns are unchanged. _explicit_ode compares the equation count against the same expanded count, otherwise a system with one array equation per array unknown skipped mtkcompile and failed later with "does not have one differential equation per unknown". New testsets in test/pod.jl and test/deim.jl reduce a system whose unknown list is the array itself (the pod one errors on main, and test/deim.jl already dies at its first MethodOfLines testset there), and one in test/full_order.jl checks that the expanded rows of a 2x2 matrix unknown follow the problem's u0. With MethodOfLines 1.6.0 and ModelingToolkit 11.46.1, all eight files of the Core group pass locally.

@ChrisRackauckas

Copy link
Copy Markdown
Member

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

Independent review (Codex CLI v0.164.0 / gpt-6-astra), reviewer session 01a1c9e2-2fc0-79a2-a7ac-9b425aa527bc, reviewed head 52d1092

  • Verdict: MERGE
  • Risk assessment: medium-low (ML4: raises the ModelingToolkit lower bound from 11.42 to 11.44; array-unknown expansion is verified by failing-before/passing-after coverage and independent analytic probes).
  • Round / slot: 1 / 1

This is a blind code-only review by a session other than the PR author's, copied below from the reviewer's report with only the grading metadata removed. The posting agent did not re-run the reviewer's commands. A MERGE verdict is advice; merging is left to the maintainers.

Risk

medium-low (ML4: raises the ModelingToolkit lower bound from 11.42 to 11.44; array-unknown expansion is verified by failing-before/passing-after coverage and independent analytic probes).

Blocking findings

None. No regression attributable to this diff was confirmed.

  • Verdict: MERGE
  • Round: 1
  • Blocking priorities: none
  • Deferred P2 issues: none

Non-blocking findings

  • src/deim.jl:32 — Confirmed pre-existing numerical limitation: requesting all four modes for a seeded, full-rank 4×13 snapshot produces a basis with norm(V'V - I) == 2.0. POD reconstructs the initial [0.1, 0.2, 0.3, 0.4] as approximately [-0.301114, 0.152839, 0.058551, 0.119669]; DEIM throws SingularException. The identical scalarized-input results reproduce on origin/main (f0ac73564251bba642fafd2b17c56cf16748c6cd). Follow up on full-rank TSVD handling, with independent orthogonality and initial-reconstruction assertions; consider dense SVD at this boundary. Reproducer: scratch/diagnostic.jl; evidence: scratch/diagnostic-{head,base}.log.
  • src/pod.jl:106 — Confirmed pre-existing shape limitation: a 2×3 field with D(q) ~ -q-q.^3 and two retained modes errors when multiplying the 2×6 projection matrix by the 2×3 residual. Matrix DEIM passes the analytic probe. Matrix POD also fails on the baseline with explicitly scalarized unknowns, so this is not introduced here. Follow up by flattening the symbolic field residual in column-major order before projection and testing matrix/tensor POD. Evidence: scratch/invariant-head.log and the baseline diagnostic.
  • AGENTS.md:25 — Confirmed by reading: “Source systems must list scalar unknowns” contradicts this PR's new supported inputs. Update that sentence while retaining the warning about registering both an array and its elements.

What I ran

AI reviewer: Codex CLI 0.164.0, model gpt-6-astra; local session identified above (no session URL).

All Julia commands used the supplied layered JULIA_DEPOT_PATH, the requested TMPDIR, capped scopes, and scratch copies. The reviewed checkout remained unchanged. No Julia manifest or interpreter lock is tracked; the resolved environment was retained for comparisons. Julia reported 1.13.2. Core resolved ModelingToolkit 11.46.1, ModelingToolkitBase 1.80.0, Symbolics 7.48.0, and SymbolicUtils 4.55.0.

  • GROUP=Core julia --project -e 'using Pkg; Pkg.test()': 252/252 passed, exit 0 (scratch/core.log). Includes all affected test files.
  • GROUP=QA julia --project -e 'using Pkg; Pkg.test()': 26/26 passed, exit 0 (scratch/qa.log).
  • julia --project=docs docs/make.jl: passed, exit 0, including doctests and document checks; deployment skipped automatically (scratch/docs.log).
  • Ran HEAD's test/full_order.jl against the baseline package with the same dependency manifest: the existing 32 assertions passed; the added array-unknown test passed its initial assertion then errored with ArgumentError: the compiled system does not have one differential equation per unknown. On HEAD, all 41 assertions passed. Evidence: scratch/regression-base.log and the Core run.
  • julia --startup-file=no --depwarn=yes --project=scratch/probe-env scratch/invariant.jl (from the review directory): independent checks against x(t)=x₀ exp(-t)/sqrt(1+x₀²(1-exp(-2t))). Two signed equal-amplitude groups form an exact two-mode invariant subspace. Vector and reordered mixed scalar/vector/singleton-field POD and DEIM, plus matrix DEIM, passed through both DAE and compiled ODE paths; maximum absolute error was below 2.2e-11 against a 1e-9 bound (100 times solver abstol=1e-11). Generated graph sizes for grids 4 and 12 were 32/32 for DEIM and 20/20 for POD. The script exited 1 solely for the pre-existing matrix POD failure.
  • Ran scratch/probes.jl and scratch/diagnostic.jl on HEAD, then the diagnostic on the baseline. These exposed and reproduced the full-rank and matrix limitations above. The one-state reduction probe also encountered the existing size(data, 1) > 1 input restriction; singleton fields within larger systems passed.
  • In a separate scratch environment, selected ModelingToolkit@11.44.0 with Pkg.add, then included test/full_order.jl and test/pod.jl: 69/69 passed, exit 0 (scratch/min.log). Transitive dependencies retained their resolved versions.
  • git diff --check origin/main...HEAD and typos over the seven changed files: exit 0. Final git status --short: empty.
  • Required uv run --locked python --version: printed Python 3.15.0 and warned that --locked has no effect outside a project. This Julia repository has no Python project/lockfile; that output is not evidence of a locked project Python interpreter. Python was not used for verification.

What I did not verify

Other Julia versions, 32-bit platforms, the complete transitive minimum-dependency combination, or remote CI. No PR body, comments, prior reviews, or author risk rating were read. No checkout edits, commits, pushes, or GitHub posts were made.


🤖 Posted by an AI agent — harness: Claude Code 2.1.322 · model: claude-sonnet-5-5
Conversation: local session 3878b54e-28f2-4a01-a176-a8f5468d1997 (no session URL)

@ChrisRackauckas

Copy link
Copy Markdown
Member

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

Independent review (devin / fusion-gpt-6-1-sol-high-sidekick-swe-2-medium), reviewer session unknown, reviewed head 52d1092

  • Verdict: MERGE
  • Risk assessment: medium-low (ML4: raises the ModelingToolkit compatibility lower bound from 11.42 to 11.44; array-unknown normalization has failing-before/passing-after numerical evidence.)
  • Round / slot: 2 / 1

This is a blind code-only review by a session other than the PR author's, copied below from the reviewer's report with only the grading metadata removed. The posting agent did not re-run the reviewer's commands. A MERGE verdict is advice; merging is left to the maintainers.

CI: green
Session: unknown

Risk

medium-low (ML4: raises the ModelingToolkit compatibility lower bound from 11.42 to 11.44; array-unknown normalization has failing-before/passing-after numerical evidence.)

Blocking findings

None attributable to this diff. The affected suites pass, and independent analytic probes confirm the newly accepted vector inputs and DEIM multidimensional inputs.

  • Verdict: MERGE
  • Round: 2
  • Blocking priorities: none
  • Deferred P2 issues: none; this review defers no blockers.

Non-blocking findings

  • src/pod.jl:106 (pre-existing limitation, confirmed by running): the array-Galerkin path multiplies the projection by a matrix/tensor residual without flattening it. For q(t)[1:2,1:2], D(q) ~ -q - q.^3, and four-row snapshots, pod(source, snapshot, 2) throws incompatible-shapes errors for (2,4) * (2,2); a (2,1,2) field also throws. DEIM handles both shapes numerically. The scalarized-matrix control throws the identical error on origin/main, so this is not a regression introduced here. Follow up by vectorizing the field residual before projection and adding multidimensional POD reconstruction tests, or explicitly document the vector-only restriction. Evidence: scratch/probe-head.log, scratch/probe-base.log.
  • AGENTS.md:25-27 (confirmed by reading): the instruction that sources must list scalar unknowns contradicts the array-valued unknown support and new [r]/[a,u,v,w] tests. Update that restriction while retaining the warning about automatically collected duplicate array/indexed unknowns.

What I ran

  • Exported TMPDIR=(a local scratch path) before execution; preserved the inherited layered JULIA_DEPOT_PATH. All generated environments, logs and probes are outside the checkout.
  • Inspected git diff origin/main...HEAD, affected implementation/tests, repository instructions and newest commit attribution trailers; ran git diff --check origin/main...HEAD. No whitespace errors. Both requested author trailers are absent. Final checkout status is clean and HEAD matches the reviewed SHA.
  • Used Julia 1.13.2 and the repository-declared Julia projects. No Julia Manifest is tracked; resolved manifests are retained in scratch. Test/probe dependencies include ModelingToolkit 11.46.1, OrdinaryDiffEq 7.8.1, SciMLBase 3.58.0, SymbolicIndexingInterface 0.3.56 and SciMLTesting 2.13.3.
  • In scratch/suite-head: GROUP=Core julia --project -e 'using Pkg; Pkg.test()' passes all 252 assertions; GROUP=QA julia --project -e 'using Pkg; Pkg.test()' passes all 26 assertions. Both exit 0. Logs: scratch/core.log, scratch/qa.log.
  • Ran scratch/adversarial_arrays.jl with julia --project=scratch/probe-head, then with --project=scratch/probe-base, using identical dependency versions. The base is f0ac73564251bba642fafd2b17c56cf16748c6cd. Head: seven probes pass and three report the inherited POD shape limitation. Base: all ten fail, with the newly supported raw-array cases rejected by snapshot row counts. The probe deliberately reports each caught exception; its process exit status alone is not a passing result.
  • Numerical reference: for x' = -x-x^3, used the independently derived solution x(t)=x0*exp(-t)/sqrt(1+x0^2*(1-exp(-2t))). Two active coordinates give an invariant two-dimensional subspace. Training ends at 0.3; reconstructed dynamics are checked through 0.7 with solver tolerances 1e-10 and an absolute-error bound 1e-7. Vector POD/DEIM, matrix/tensor DEIM and mixed scalar/vector POD have maximum errors between 8.4e-12 and 3.94e-11. Raw-array generated residual tree sizes are unchanged between grids 4 and 12: POD [20,20], DEIM [32,32]. Script and full outputs are retained in scratch/adversarial_arrays.jl, scratch/probe-head.log, scratch/probe-base.log.
  • In the scratch checkout: julia --project=docs docs/make.jl exits 0; deployment skips outside CI. The initial git-less scratch copy required its Git metadata for Documenter. Final log: scratch/docs.log.
  • Ran exactly uv run --locked python --version in the scratch checkout: exit 0, Python 3.15.0, with warning: "--locked has no effect when used outside of a project". This Julia repository has neither pyproject.toml nor uv.lock; that output does not establish a locked project Python environment. No system-Python fallback was invoked. Log: scratch/python-version.log.

What I did not verify

  • Julia 1.10, 32-bit platforms, or the exact new ModelingToolkit minimum 11.44; local runs use Julia 1.13.2 and ModelingToolkit 11.46.1.
  • Wall-clock compilation scaling: the targeted graph-size checks measure symbolic trees, not compilation time.
  • Remote CI was supplied as green, not independently queried. No PR body, comments, prior reviews, sibling review directories or author ratings were read. No commits, pushes, GitHub comments or checkout edits were made. All started processes have completed.

🤖 Posted by an AI agent — harness: Claude Code 2.1.322 · model: claude-sonnet-5-5
Conversation: local session 3878b54e-28f2-4a01-a176-a8f5468d1997 (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.

deim/pod reject MethodOfLines ≥1.6 discretizations: array-valued unknowns vs scalar snapshot rows (main red)

2 participants