Repository navigation
Count snapshot rows from scalar unknowns so array-valued unknowns reduce - #218
singhharsh1708 wants to merge 2 commits into
Conversation
Independent review (Codex CLI v0.164.0 / gpt-6-astra), reviewer session 01a1c9e2-2fc0-79a2-a7ac-9b425aa527bc, reviewed head 52d1092
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. Riskmedium-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 findingsNone. No regression attributable to this diff was confirmed.
Non-blocking findings
What I ranAI reviewer: Codex CLI 0.164.0, model gpt-6-astra; local session identified above (no session URL). All Julia commands used the supplied layered
What I did not verifyOther 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 |
Independent review (devin / fusion-gpt-6-1-sol-high-sidekick-swe-2-medium), reviewer session unknown, reviewed head 52d1092
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 Riskmedium-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 findingsNone attributable to this diff. The affected suites pass, and independent analytic probes confirm the newly accepted vector inputs and DEIM multidimensional inputs.
Non-blocking findings
What I ran
What I did not verify
🤖 Posted by an AI agent — harness: Claude Code 2.1.322 · model: claude-sonnet-5-5 |
Fixes #216. MethodOfLines 1.6 keeps each discretized dependent variable as one array-valued unknown, so
length(unknowns(sys))no longer counts solution rows anddeim/podrejected its problems withDimensionMismatch(two unknowns against twelve rows intest/deim.jl). The row counts indeim,podandFullOrderModelnow come fromscalar_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_odecompares the equation count against the same expanded count, otherwise a system with one array equation per array unknown skippedmtkcompileand failed later with "does not have one differential equation per unknown". New testsets intest/pod.jlandtest/deim.jlreduce a system whose unknown list is the array itself (thepodone errors on main, andtest/deim.jlalready dies at its first MethodOfLines testset there), and one intest/full_order.jlchecks that the expanded rows of a 2x2 matrix unknown follow the problem'su0. With MethodOfLines 1.6.0 and ModelingToolkit 11.46.1, all eight files of the Core group pass locally.