Repository navigation
Reject observable-space data at HamiltonianSINDy's entry point - #34
Merged
Merged
Conversation
`TrainingData` is also the name of `ReducedComplexityModeling`'s type for data on a solution space, so a session that `using`ed both packages resolved the name to neither and could use neither. The type keeps its fields, constructors, selectors and accessors; the name is the only thing that changes, and the package has never been registered. The bridge that consumes a `ReducedComplexityModeling` `TrainingData` is deliberately not in this commit: `SparseIdentification` cannot yet take that package as a dependency, because `ReducedComplexityModeling` 0.2.0 requires `AbstractNeuralNetworks = "0.6"` while every `GeometricOptimizers` this package admits declares a weak dependency on `AbstractNeuralNetworks = "0.7"` or `"0.8"`, so no environment can hold both. `TrajectoryData`'s and `HamiltonianSINDy`'s docstrings now state that an untagged state is taken to be canonical `(q, p)`, and `KNOWN_ISSUES.md` gains K10 for the `TrajectoryData` `Δt` that the fit does not read.
ReducedComplexityModeling tags its training data with the space the data live in. A `TrainingData` on an `IntrinsicSpace` whose states are `StateData` on a `CanonicalHamiltonianSystem` is exactly what the Hamiltonian flow-map fit assumes, so `identify` now builds the state pairs from it — a `d×N` matrix of consecutive states, or RCM's `d×M×N` ensemble with consecutive times paired within each member — and forwards to the unchanged `TrajectoryData` path. Every other `TrainingData` is refused at the entry point with an `ArgumentError` naming the method, the space, the data type and the system type, as this package does for every unsupported (problem, method) pair, so observable-space data is never fitted in silence. The states of a pair must be one step of `method.integrator_timestep` apart, to `√eps`, because `sparsify` integrates the flow map with the method's step and never reads the data's (K10). A `TrainingData` built at another step is refused rather than fitted for the interval it does not describe; one whose times accumulated round-off is at the step and fits. `ReducedComplexityModeling` becomes a dependency at `"0.3"`. Version 0.2.0 could not be held beside any `GeometricOptimizers` this package admits: it required `AbstractNeuralNetworks = "0.6"` while 0.7.0 and 0.8.0 both declare a weak dependency on `"0.8"`. Nothing is re-exported and the conversion is private — no `TrajectoryData(::TrainingData)` method and no new public name, so `TrainingData` stays RCM's type and `TrajectoryData` stays this package's. `test/reducedcomplexitymodeling.jl` covers the five checks, and asserts the identified coefficients of the harmonic oscillator as ½ on `q²` and on `p²` with every other coefficient zero. That is the assertion that kills both mutants K8 records, so K8 leaves `KNOWN_ISSUES.md` and the CHANGELOG names it.
The suite was red: `test/reducedcomplexitymodeling.jl` imports `LinearAlgebra` and `test/Project.toml`
had no entry for it. `Pkg.test()` gives the test project only that file's dependencies plus the
package and takes `@stdlib` off the load path, so the run died at the import while a named selection
of the same file passed. The entry is added with no `[compat]` line, since the package already
bounds it. `Pkg.test()` now passes: Aqua 9, Basis 30, Extended Bases 60, Training Data 15, Solvers
11, SINDy 12, Hamiltonian SINDy 31, Reduced Complexity Modeling 52, JuliaGNI Conformance 49.
`_check_timestep` took its tolerance from the container's *declared* element type, so a time vector
that is valid by every other rule — `AbstractFloat[0.0, 0.01]`, `Any[0.0, 0.01]` — raised
`MethodError(eps, (AbstractFloat,))` or `MethodError(float, (Any,))`. The tolerance now comes from
the floating type of the step *values*. The conversion also stores `method.integrator_timestep`
rather than the first difference of the time vector, which for times that start away from 0 is the
method's step only to round-off, so `timestep` is the interval the fit integrates over.
Four mutants survived the round-1 tests, all of them in the tests rather than in the code, and each
now dies:
- the refusal's method name is asserted as `"… cannot be applied"` and not as a substring that the
message's fixed text ("`HamiltonianSINDy` fits the flow map…") also holds;
- the refusal's "map the data to canonical `(q, p)` and tag them `IntrinsicSpace`" instruction is
asserted too, where `occursin("IntrinsicSpace", …)` was satisfied by the pair the message names;
- one interior irregular interval is refused, in a time vector whose first interval is at `Δt`,
which pins `all` over every interval against both `any` of them and the first one alone;
- a step `1e-4` relative away is refused, which pins the `√eps` bound against a tolerance of a few
percent.
An ensemble with no member is refused like the other layouts instead of reaching a `BoundsError`
from the regression, and a time vector whose own step is the method's only to `√eps` is asserted to
carry the method's step into the conversion rather than its own. The comment on the `TrainingData`
name states which package owns which name and why the two are named apart, in the present tense.
The `identify` docstring's table lists the new accepted pair, and K9's citation of `method.jl` moves
with the line it gained. `src/reducedcomplexitymodeling.jl`'s header goes: every clause of it — the
accepted axes, the refusal, the space as the load-bearing parameter, the privacy of the conversion —
is stated in the docstrings below it. K10's evidence is a command and its output rather than a
reading of the code.
Both blocking defects the second round found were evidence, so the fix is in the tests. The layout guards in `test/reducedcomplexitymodeling.jl` each assert their own phrase beside `isa ArgumentError`: the 1-D input `"is 2 or 3, got 1"`, the 4-D input `"is 2 or 3, got 4"`, the single time in either layout `"at least two times"`, and the no-member ensemble `"at least one member"`. Without the phrase, a dropped guard is answered by a neighbouring refusal — N14's missing two-times check fell through to the no-state-pair guard, the same `ArgumentError` — so the test could not tell which guard refused. The two refusals assert the method's name with `startswith(err.msg, …)`: the observable-space refusal `"HamiltonianSINDy cannot be applied"` and the pair refusal `"SINDy cannot be applied"`. `message(err)` is `sprint(showerror, err)`, whose text begins `"ArgumentError: "`, so `startswith` on it fails; `err.msg` is the guard's own sentence. A refusal that hard-codes `HamiltonianSINDy` (V1) now fails the `SINDy` assertion. The time-step testset fits a `Float32.(time)` vector and asserts `refusal(...) === nothing`: each of its steps sits a relative ~2e-8 from `Δt`, which the tolerance from `Float32`'s `eps` (√eps ≈ 3.5e-4) takes and `√eps(Float64)` (1.5e-8) refuses, so a tolerance taken from `Float64` whatever the values (V4) is caught. V5, the tolerance from the first step's type alone, is the permitted survivor: it behaves exactly as the head on any time vector whose values share one type, and §5V.P25 decides no vector that mixes value types. K10's evidence field carries the commands that were run and their output: the grep prints five lines (3, 37, 44, 48, 154), and the refusal is reported as the separate `timestep(TrajectoryData(x, y, 0.02))` → `0.02` and `…integrator_timestep` → `0.01` calls, not as a printed result line that nothing prints. No file under `src/` changes.
K11: `_check_timestep`'s tolerance is not inferable, so the routine allocates on every storage type; K12: `steps = diff(time)` materialises a vector that is only traversed; K13: the comment over `_state_pairs` describes the flattening wrongly. All three are in the new `src/reducedcomplexitymodeling.jl`, which the arbitrator froze for the test round, so they are recorded rather than fixed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #34 +/- ##
==========================================
+ Coverage 89.05% 89.91% +0.86%
==========================================
Files 9 10 +1
Lines 338 357 +19
==========================================
+ Hits 301 321 +20
+ Misses 37 36 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
michakraus
commented
Oct 16, 2026
michakraus
left a comment
michakraus
left a comment
Member
Author
There was a problem hiding this comment.
JuliaRCM/SparseIdentification.jl PR #34 — Reject observable-space data at HamiltonianSINDy's entry point
Verdict: request changes (posted as a comment review)
Reviewed: 4cb8f20b1198411e97db2df6383225dbe7d60704
CI: the seven required checks are green. Downgrade - ubuntu-latest is red, but the job is advisory (continue-on-error: true, line 135 of CI.yml) and it was red on main before this PR. Details are below the findings.
The focus of this review is code quality. The new bridge, src/reducedcomplexitymodeling.jl, is 145 lines, and about 55 of them carry the behaviour. A version of about 60 lines, docstring included, passes all 58 assertions of this PR's own test/reducedcomplexitymodeling.jl unchanged (see F1). The PR also lists three defects in that new file as K11 to K13 in KNOWN_ISSUES.md and does not fix them. The replacement in F1 fixes all three.
Findings
| # | severity | file:line | claim | evidence |
|---|---|---|---|---|
| F1 | quality | src/reducedcomplexitymodeling.jl:101-145 |
The conversion is about twice as long as it needs to be. It also ships K11, K12 and K13, which the PR records but does not fix. _check_timestep re-implements the default of isapprox. rtol = √eps(float(mapreduce(typeof, promote_type, steps))) (:136) does what isapprox(step, Δt) does without rtol: Base uses max(√eps(typeof(step)), √eps(typeof(Δt))) there. The times ≥ 2 guard (:109-112) can never fire on its own, because fewer than two times leaves x empty and the isempty(x) guard at :115 already catches it. The ndims guard (:104-107) can be a fallback method. The replacement is below the table. |
The PR check and all(≈(Δt), diff(time)) agree on all 15 cases I ran: Float64, (1+1e-9)t, an irregular interior step, a 2Δt method, a (1+1e-4)Δt method, cumsum, AbstractFloat[], Any[], Float32, a range, Int times, (1+1e-7)t, (1+3e-8)t, Float32 with 1e-3 and with 1e-4. isempty(first(_state_pairs(...))) is true for a 2×1 matrix and for a 2×2×1 array. I installed the replacement with @eval SparseIdentification and ran this PR's test file against it: 58 of 58 pass. |
| F2 | bug | src/reducedcomplexitymodeling.jl:64-71 |
The refusal gives the wrong advice when the method is SINDy. On data that are already canonical and tagged IntrinsicSpace, it says "map the data to canonical (q, p) and tag them IntrinsicSpace", which the caller has already done. The second sentence also names HamiltonianSINDy, whatever the method is. |
identify(TrainingData{IntrinsicSpace, StateData, CanonicalHamiltonianSystem}(t, X), SINDy(CompoundBasis(polyorder = 2); λ = 0.05)) raises SINDy cannot be applied to a TrainingData{IntrinsicSpace, StateData, CanonicalHamiltonianSystem}. HamiltonianSINDy fits the flow map of canonical (q, p) states, so it takes ... and nothing else: map the data to canonical (q, p) and tag them IntrinsicSpace. |
| F3 | correctness-risk | test/reducedcomplexitymodeling.jl:152-155, test/methods/hamiltonian.jl |
K8 is closed only by the test file of the RCM bridge. A test of the sign and the order in src/methods/hamiltonian.jl belongs in test/methods/hamiltonian.jl. As it is, removing the bridge or moving it to an extension (F4) brings K8 back without a sign. |
I applied both K8 mutants to a copy of the head. f_expr = ∇H[1:d]: test/methods/hamiltonian.jl PASS, test/reducedcomplexitymodeling.jl FAIL. vcat(f_expr, v_expr): PASS and FAIL. The unmutated copy: PASS and PASS. Fix: copy the four coefficient assertions (a[3] ≈ 0.5, a[5] ≈ 0.5, all(iszero, a[[1, 2, 4]]), nterms == 2) into test/methods/hamiltonian.jl, on a TrajectoryData(zs[1:end-1], zs[2:end], Δt) that is built directly. |
| F4 | quality (design decision) | Project.toml [deps] |
ReducedComplexityModeling becomes a hard dependency only to serve a bridge. Every user then pays for its dependencies: HDF5 and HDF5_jll, the MPI jlls, Optim, TypedTables, KernelAbstractions and others. A caller who holds an RCM TrainingData has already loaded RCM, so a package extension would change no API. |
The manifest gains RCM and its dependency cone, among them the HDF5, MPI and aws jlls. Cold @elapsed using SparseIdentification, three runs each, on a loaded machine (load average 23): base 1.76 / 1.98 / 1.74 s with 162 modules, head 2.32 / 2.36 / 2.27 s with 221 modules. If the maintainer chooses the extension: move RCM to [weakdeps], add [extensions] SparseIdentificationReducedComplexityModelingExt = "ReducedComplexityModeling", and move the file to ext/ inside module SparseIdentificationReducedComplexityModelingExt, with using SparseIdentification, ReducedComplexityModeling and import SparseIdentification: identify. The tests then reach _trajectory_data through Base.get_extension. |
| F5 | quality | src/reducedcomplexitymodeling.jl:4-63, docs/src/usage/ecosystem.md:73-87, src/methods/method.jl:75 |
The same contract is written out five times: the docstring of the accepting method (42 lines), the docstring of the refusal, the new section of ecosystem.md, the CHANGELOG, and the inline comments at :121-122 and :126-132. ecosystem.md:26 already renders every identify docstring with @docs, so that page shows the contract three times. Fix: delete the docstring of the refusal (:53-63) and the docstring of the private _trajectory_data (:93-100), shorten the docstring of the accepting method to the 12-line version below, and cut the ecosystem.md section to two sentences that point to identify. |
Read at the cited lines. |
| F6 | quality | test/reducedcomplexitymodeling.jl:45-66, 98-117, 157-170, 73-83 |
Much of the test file tests itself and not the package. The testset "The observation is not a canonical coordinate pair" (:98-117) checks only the Jacobian of the test's own observe, and it is the only reason for using LinearAlgebra and the new LinearAlgebra entry in test/Project.toml. The plain-fit half of :157-170, together with flow_map_loss (:49-66), goes through no bridge code: the coefficient assertions at :152-155 already decide the composed fit. Line :183-184 checks the fixture's time vector. Line :95 repeats :93. The helpers refusal and message (:73-83) re-implement @test_throws with a message pattern. |
@test_throws ["HamiltonianSINDy cannot be applied", "ObservableSpace"] f() and @test_throws r"^ArgumentError: HamiltonianSINDy cannot be applied" f() both return Test.Pass on Julia 1.11.9. A wrong pattern gives a Test Failed that shows the message. Replacement for :127-136: @test_throws r"^ArgumentError: HamiltonianSINDy cannot be applied" identify(data, method) and @test_throws ["ObservableSpace", "StateData", "CanonicalHamiltonianSystem", "map the data to canonical", "tag them"] identify(data, method). Use the same shape for the layout guards, for example @test_throws r"is 2 or 3, got 1" identify(...). :235-238 becomes @test identify(..., method) isa HamiltonianSINDyResult. With observe used only in the refusal testset, where the numbers do not matter, use states there and delete observe. |
| F7 | quality | Project.toml [compat] GeometricBase = "0.14" |
RCM 0.3.0 declares GeometricBase = "0.14.6, 0.15", so this PR adds a second reason why the floor 0.14 cannot hold. Downgrade already names this entry on main. Fix: raise the floor to at least "0.14.6", or to "0.14.9", the version that forcedeps resolves. |
ReducedComplexityModeling 0.3.0 Project.toml: GeometricBase = "0.14.6, 0.15". Downgrade log on this PR: forcedeps check failed: GeometricBase resolved to 0.14.9 but lower bound is 0.14.0. |
| F8 | quality (design decision) | src/methods/hamiltonian_sindy.jl:154, KNOWN_ISSUES.md K10 |
The step check exists only in the bridge. identify(::TrajectoryData, ::HamiltonianSINDy) still fits silently when problem.Δt != method.integrator_timestep. The PR's own K10 says so: "A caller who reaches the fit through an entry point that checks the step is protected; nothing else is." If the maintainer makes identify(::TrajectoryData, ::HamiltonianSINDy) throw when timestep(problem) ≉ method.integrator_timestep, the check sits in one place and covers both entry points, and the bridge keeps only the uniformity test all(≈(Δt), diff(time)). This changes behaviour for existing callers of TrajectoryData, so it is a decision for the maintainer. |
KNOWN_ISSUES.md K10, as added by this PR. |
| F9 | nit | src/reducedcomplexitymodeling.jl |
The bridge depends on implicit imports. StateData and CanonicalHamiltonianSystem come in through using GeometricBase, and only the new file uses them. Fix: add using GeometricBase: CanonicalHamiltonianSystem, StateData beside the imports at :1-2. |
print_explicit_imports(SparseIdentification) lists both names under using GeometricBase: .... A text search finds them only in src/reducedcomplexitymodeling.jl. |
| F10 | nit | src/reducedcomplexitymodeling.jl:105,116,139 |
$(nameof(typeof(method))) is interpolated where the signature already fixes method::HamiltonianSINDy. Write the name as a literal. The generic fallback at method.jl:98 spells the same thing typeof(method).name.name. |
Read at the cited lines. |
| F11 | nit | src/reducedcomplexitymodeling.jl:121,127, KNOWN_ISSUES.md K11 |
The source comments cite K10, an ID of KNOWN_ISSUES.md, and that citation goes stale when K10 closes. K11 cites "§L", which a reader of this repository cannot resolve. If F1 is applied, K11 to K13 go away. |
Read at the cited lines. |
| F12 | nit | scripts/*.jl (8 sites) |
Eight TrainingData( call sites in scripts/ still use the old name after the rename. K7 already marks these scripts as broken, so they do not block the merge. |
A text search for TrainingData under scripts/ |
F1 and F2, the replacement for src/reducedcomplexitymodeling.jl. This code passes the PR's test file unchanged: 58 of 58.
import ReducedComplexityModeling as RCM
using ReducedComplexityModeling: IntrinsicSpace
using GeometricBase: CanonicalHamiltonianSystem, StateData
"""
identify(problem::ReducedComplexityModeling.TrainingData, method::HamiltonianSINDy; kwargs...)
Identify the Hamiltonian of a `ReducedComplexityModeling.TrainingData{<:IntrinsicSpace, StateData,
CanonicalHamiltonianSystem}`, whose states are canonical `(q, p)`, `q` first.
The states are along the last dimension: a `d×N` matrix is one trajectory, a `d×M×N` array holds
`M` of them, and consecutive times are paired within each member. The times must be one
`method.integrator_timestep` apart, since the fit integrates at that step and never reads the
data's. Every other `TrainingData` raises an `ArgumentError` that names its space, data type and
system type.
"""
function identify(
problem::RCM.TrainingData{<:IntrinsicSpace, StateData, CanonicalHamiltonianSystem},
method::HamiltonianSINDy; kwargs...)
identify(_trajectory_data(problem, method), method; kwargs...)
end
function identify(problem::RCM.TrainingData{ST, DT, SY}, ::HamiltonianSINDy;
kwargs...) where {ST, DT, SY}
throw(ArgumentError(
"HamiltonianSINDy cannot be applied to a TrainingData{$ST, $DT, $SY}: it fits the " *
"flow map of canonical `(q, p)` states, so map the data to canonical `(q, p)` and " *
"tag them `IntrinsicSpace`."))
end
# A `d×M×N` ensemble is paired within each member; reshaping it to `d×(M·N)` first would
# interleave the members and pair member `i` with member `i + 1` at the same time.
function _state_pairs(data::AbstractArray{<:Any, 3})
m, n = size(data, 2), size(data, 3)
([view(data, :, i, j) for i in 1:m for j in 1:(n - 1)],
[view(data, :, i, j + 1) for i in 1:m for j in 1:(n - 1)])
end
_state_pairs(data::AbstractMatrix) = _state_pairs(reshape(data, size(data, 1), 1, size(data, 2)))
_state_pairs(data::AbstractArray) = throw(ArgumentError(
"the states of a TrainingData are `d×N` or `d×M×N`, so `ndims(data)` is 2 or 3, " *
"got $(ndims(data))"))
function _trajectory_data(problem::RCM.TrainingData, method::HamiltonianSINDy)
x, y = _state_pairs(problem.data)
isempty(x) && throw(ArgumentError(
"the states of shape $(size(problem.data)) form no state pair: a fit needs " *
"at least two times and at least one member"))
Δt = method.integrator_timestep
steps = diff(problem.time)
all(≈(Δt), steps) || throw(ArgumentError(
"`diff(time)` runs from $(minimum(steps)) to $(maximum(steps)), but HamiltonianSINDy " *
"integrates at `integrator_timestep = $Δt` and never reads the step of the data"))
TrajectoryData(x, y, Δt)
endFor F2, the refusal is restricted to HamiltonianSINDy. SINDy on an RCM TrainingData then reaches the package's generic refusal, after one change at src/methods/method.jl:97. Change function identify(problem::IdentificationProblem, method::SparsificationMethod; kwargs...) to function identify(problem, method::SparsificationMethod; kwargs...). That refusal reads "SINDy cannot be applied to TrainingData. See the documentation for which formulation each method needs.", which is correct, and the PR's startswith(err.msg, "SINDy cannot be applied") still passes. detect_ambiguities reports no identify pair on the head; I did not run it on the replacement.
The reshape makes a d×N matrix a one-member ensemble, so one pairing method serves both layouts. For an Array, reshape shares memory and returns an Array, so the views stay plain strided views. If you prefer the two explicit methods of the PR, keep them. The other changes do not depend on this one.
After F1, remove K11, K12 and K13 from KNOWN_ISSUES.md. K11 is about the rtol line, which no longer exists. K12 is not worth an entry: diff runs once per identify call on N-1 numbers. K13 is fixed by the replacement comment.
CI
- Required checks, from
repos/.../branches/main/protectionandrepos/.../rules/branches/main(ruleset 24118240). The two agree on seven contexts:Julia minandJulia 1on each of ubuntu, macOS and windows, andDoctests - ubuntu-latest. All seven are green on4cb8f20. Downgrade - ubuntu-latest(run 38870868714, job 116874376156) fails atjulia-downgrade-compatand never reaches the tests:ERROR: LoadError: forcedeps check failed for .: Some packages did not resolve to their lower bounds.The failing entries areSimpleSolvers resolved to 0.13.2 but lower bound is 0.13.0,GeometricBase resolved to 0.14.9 but lower bound is 0.14.0andRuntimeGeneratedFunctions resolved to 0.5.12 but lower bound is 0.5.0. The last CI run onmain(37980887521,7fa8748, 2026-10-09) failed this job on the same three entries, with RuntimeGeneratedFunctions at 0.5.9. The failure is older than this PR. The new entry passes:forcedeps check passed: ReducedComplexityModeling = 0.3.0. F7 is the one way this PR makes the GeometricBase floor more wrong. Themainbaseline is a week old, and I did not dispatch a new one, because the three failing entries are floors that this PR does not change.codecov/patchandcodecov/projectpass.
Verified and fine
test/reducedcomplexitymodeling.jlon the head, in a scratch environment that develops the worktree (RCM 0.3.0, Julia 1.13): 58 of 58 pass. This matches the count in the PR body.Aqua.test_all(SparseIdentification)on the head: 11 of 11 pass.detect_ambiguitiesreports noidentifypair.- The ensemble pairing within each member is correct. The PR test file asserts the coefficients ½ and ½ on a two-member
2×2×41ensemble, and this passes. - The K13 claim holds. Column-major flattening interleaves the members; it does not join their ends. This follows from reading
_state_pairsand the layout. The replacement comment in F1 says this. - No
close,fixorresolvekeyword appears before an issue number:closingIssuesReferencesis empty. - The PR body carries a "Verdict: PASS" from the author's own process ("the part's last critic round") and names its roles (advisor, arbitrator). This review treats that as a claim, not as evidence. It does not change the verdict.
Not checked
- The full
Pkg.test(), as instructed. The seven required CI jobs ran it. - The suite at the dependency floors. Downgrade stops at
forcedepsonmainand on this PR, so nothing tests the floors. - The K11 allocation figures (144 B, 96 B, 800 B). The replacement removes the line they describe, so I did not measure them again.
- A local docs build. The
DocumentationCI job is green.
Resolves the review findings F1, F2, F5, F6 (in part), F9, F10 and F11. - F1: the conversion drops the unreachable times >= 2 guard, turns the ndims guard into a fallback method of _state_pairs, pairs a d x N matrix as a one-member ensemble, and computes the step tolerance per step from its own floating type, so it infers (K11). The comment over _state_pairs now says what flattening an ensemble does (K13). K12 is dropped: diff runs once per call on N-1 numbers. K11-K13 leave KNOWN_ISSUES.md. - F2: the bridge's refusal is restricted to HamiltonianSINDy, and the generic refusal in method.jl takes any problem, so SINDy on an RCM TrainingData is no longer told to re-tag data that are canonical. - F5: the refusal and private docstrings go, the accepting docstring and the ecosystem.md section are shortened. - F6: the refusal/message helpers become @test_throws with a pattern; a duplicate assertion and a fixture check go. - F9-F11: explicit GeometricBase imports, literal method names, no KNOWN_ISSUES IDs in source comments. The CHANGELOG sentence on refused TrainingData is scoped to HamiltonianSINDy and names SINDy's generic refusal. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Member
Author
Resolution of the review findingsThe review above was of
Checks run locally on |
… three floors (#34) The user's decisions on the review findings F3, F7 and F8 (F4: no change). - F8: identify(::TrajectoryData, ::HamiltonianSINDy) refuses a TrajectoryData whose step is not integrator_timestep to sqrt(eps) of its own type, so existing TrajectoryData callers now get the check. The RCM bridge checks only that the times are evenly spaced and carries the data's step. This fixes K10, which leaves KNOWN_ISSUES.md. The docstring example in geometricequations.jl passes integrator_timestep = dt. - F3: the harmonic-oscillator testset in test/methods/hamiltonian.jl asserts the identified coefficients; both K8 mutants are caught by that file alone. - F7: GeometricBase 0.14.9, RuntimeGeneratedFunctions 0.5.12 and SimpleSolvers 0.13.2 are the floors, the versions the Downgrade job resolved. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…he floors (#34) With the three floors of the previous commit, the Downgrade job's forcedeps check passes, but Pkg.test cannot resolve the test environment: beside SimpleSolvers 0.13.2, the test-only GeometricIntegrators 0.18 can only be 0.18.4-0.18.5, which need GeometricSolutions 0.6.5 or later. A local resolve with every dependency at its floor fails at GeometricSolutions 0.6.0 and succeeds at 0.6.5. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Member
Author
Follow-up: F3, F4, F7 and F8 are decidedCommits
The CHANGELOG records the step check as a behaviour change for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
SparseIdentificationrefuses observable-space data at the entry point, by dispatch on the space parameter, instead of fitting it in silence. A new bridge filesrc/reducedcomplexitymodeling.jlholds twoidentifymethods: one forRCM.TrainingData{<:IntrinsicSpace, StateData, CanonicalHamiltonianSystem}, and one for any otherRCM.TrainingDatathat throws, naming the space and telling the caller to map the data to canonical(q, p)and tag itIntrinsicSpace.TrainingDatais renamed toSINDyData, so the type no longer collides with ReducedComplexityModeling's.Verdict
The part's last critic round: Verdict: PASS. All six required mutants are CAUGHT, each dying at the assertion it is meant to hit, and no new assertion passes for the wrong reason.
startswith(err.msg, …)attest/reducedcomplexitymodeling.jl:131and:343:279/:287, and N14 at:297/:300, where the previous round'sisemptyguard had masked itFloat32.(time)fit at:235√eps(Float64)3, 37, 44, 48, 154; the two calls print0.02and0.0111 CAUGHT, 0 SURVIVED of 11src/is untouched through the fix rounds:git diff d3c286d -- src/is 0 bytes. V5 survives, as the ruling permits, and equals the head on every time vector whose values share one type.Checks
run-tests.jl <worktree> full: exit 0,PASS in 55 s; theReduced Complexity Modelinggroup 58.Recorded, not fixed
The pre-pull-request branch verification, run at the tree's requirement, left three findings in the new
src/reducedcomplexitymodeling.jl. All three are recorded as K11–K13 rather than fixed, because the arbitrator had frozensrc/for the test round; none binds §L's type-stability rule, which binds a method a time step calls, not an offline setup routine. The eightTrainingDatacall sites underscripts/are K7's and are handled separately.Decisions the part carries
TrainingDatatoSINDyData, so SparseIdentification's own type no longer collides with RCM's;identifyis kept as the entry point rather than becoming a method of RCM'strain;src/reducedcomplexitymodeling.jl, with its two methods;test roundthat fixed the tests alone;src/findings recorded with their measurements, and the scripts left to their own task.