Skip to content

Part M6: small API corrections for GeometricMachineLearning 0.9 - #358

Merged
michakraus merged 12 commits into
mainfrom
simplify/part-m6
Oct 26, 2026
Merged

michakraus merged 12 commits into
mainfrom
simplify/part-m6

Conversation

@michakraus

Copy link
Copy Markdown
Member

Part M6 of the 0.9 simplification: small API corrections

Part M6 of the plan that simplifies GeometricOptimizers and GeometricMachineLearning for release
0.9. It fixes a set of small user-visible defects, four of the fixes break an API, and two
behaviours change without a name changing. 67 files, +454 −640.

Relates to #293. Relates to #79.

What changes

Breaking, four.

  1. PoissonTensor is removed, with ext/GPUArraysCoreExt.jl, the [weakdeps] and
    [extensions] entries of Project.toml, and GPUArraysCore itself. A site that needs the
    canonical tensor builds [0 I(n); -I(n) 0] of n2 × n2 with n = n2 ÷ 2; a site that used the
    2-form writes v₁.q' * v₂.p - v₁.p' * v₂.q, because a Matrix is not callable on one argument.
  2. The seven network trajectory methods move from Base.iterate onto GeometricBase.integrate,
    and the export moves with them.
    75 call sites in 28 files, the two @docs entries and the two
    @refs renamed. GeometricIntegrators.integrate is the same function object, so a page that
    loads both packages resolves one verb.
  3. update! and solve! are no longer exported. Both keep their binding here — update! is
    GeometricBase.update!, solve! is SimpleSolvers.solve! — and the
    solve!(::NeuralNetwork{<:PSDArch}, …) methods this package defines stay. The import of update!
    stays as well: without it the name resolves through using ProgressMeter to
    ProgressMeter.update!, which does export it.
  4. The three HRedSys constructors require integrator, so a call that names none raises
    UndefKeywordError(:integrator), and the positional constructor defaults parameters to
    NullParameters(). ImplicitMidpoint is named nowhere in src/.

Fixed, two.

  • accuracy returns the element type of the loader's input. It divided an Int by an Integer,
    so / made it a Float64 for every loader, and a Float64 model widened a Float32 loader's
    result. The value of the fraction does not move.
  • map_to_cpu returns a network whose backend is the host's: the CPU it was handed, or CPU().
    It returned host leaves behind a device backend and a device type parameter, so the next forward
    pass sent host data to the device the network came from.

Elsewhere. _seq_length replaces the ternary the two matrix methods wrote twice;
InteractiveUtils and TimerOutputs leave [deps] and [compat]; test/quality/exports.jl gains
the four names that must not be exported; the tutorials, the scripts and the manual's GPU section
follow the removals.

The loop

  • Round 1, build 1 at f727c82d stopped on the first advisor question, with head_facts.jl
    green and 3 of 12 mutants run.
  • Advisor, 2026-10-26: the two decisions below, neither parked. Build 2 at 7412b05c:
    head_tree.sh green, head_facts.jl 40 of 40, 15 mutants written, 14 caught, 1 equivalent.
  • Round 1, two blind critics, both FAIL on 7412b05c, 7 blocking defects: the
    symplectic_autoencoder tutorial's solve! outside its @example block, docs/src/index.md's
    "three testsets", the two new testsets' precision loops and allowscalar(false), three comments
    that told history, the accuracy test that could not fail for the element type it names, and the
    sentence naming tensor_cayley.
  • Six spec clauses were corrected before the fix round, five of them defects of the spec's own
    text.
  • Fix 1 at de7c7bc1; verify round 2: PASS, no blocking defect, no other defect of the fix
    diff, all nine mutants caught.
  • Finish pass. A line-by-line read of the branch's text, then three julia-branch-verifier
    passes. Four commits follow de7c7bc1, all text: e3ffd50b corrects one test comment, 63ccb563
    the rationale for import GeometricBase: integrate, and 06ee50fc and 47f534bc the changelog —
    two ### Fixed bullets for this part's two behaviour changes, and four [Unreleased] bullets
    brought to the release's end state, since an open section is not history.

Gates

  • Critic verify 2: PASS — verdict and evidence table under Critic below.
  • Whole suite at de7c7bc1: 93 testsets, 0 failed — Julia 1.13.2 in 448 s, Julia 1.12.7 in
    679 s.
  • Spec gate at 47f534bc: spec-validate.py --head PASS — head_tree.sh and head_facts.jl
    both pass.
  • test/quality/jet.jl: PASS, 50 assertions, 6 s — a named run on Julia 1.13.2 of the tree at
    63ccb563, whose two successors change CHANGELOG.md only.
  • CI at de7c7bc1: CI success (run 40313975979 — Doctests, Julia 1, Julia min on Ubuntu,
    macOS and Windows); Documentation success (40313979594); Metal success (40313983268); Scripts
    success (40313987455). Downgrade, Julia pre and Julia nightly are red on main at
    d7a488c2 as well, so they are not this part's.
  • The four commits after de7c7bc1 change a comment, one test comment and CHANGELOG.md; no
    executable line changes, so CI is not re-run for them.

Pre-PR verification

Pass 3 of 3, julia-branch-verifier — verdict: clear to open, 0 unresolved. Diff
a664f5c9...47f534bc, 67 files, +454/−640, 8 commits. The last change to src/ is 63ccb563 (one
comment) and to test/quality/jet.jl is 0e1136b8, so the passes re-judged points 1, 6 and 7 on
the rewritten lines; points 2–5 stand as measured in pass 1.

Seven points over the whole diff. 1 · correctness, quality, concision — every changed hunk read
with context; integrate is GeometricBase.integrate with 7 methods and no iterate left;
_seq_length covers the three transformer families that have no seq_length field; every
HRedSys( call site passes the now-required integrator. 2 · type piracy —
Aqua.test_piracies passes; the added methods are on this package's NeuralNetwork for
GeometricBase.integrate, and the PoissonTensor extension is deleted with the type.
3 · duplicated functionality — nothing re-implements a dependency: mapstorage is
NeuralNetworkParameters', integrate GeometricBase's, the triangular types GeometricOptimizers'.
4 · type instability — code_typed gives concrete returns for _seq_length on all three
architectures, map_to_cpu(::NeuralNetwork) and accuracy; no Any originates in a changed hunk.
5 · avoidable allocations — fatou lint over 154 files clean. 6 · stale comments — every
added comment and docstring line read against its code, present tense. 7 · history only in
CHANGELOG.md
— historical phrasing occurs nowhere else, and [Unreleased] carries entries for
both behaviour changes of this part. Also clean: the four enabled ExplicitImports checks, Aqua
stale_deps, detect_ambiguities (0 recursive, 0 non-recursive), and the formatter over the 49
changed .jl files.

Pass 1 fixed the rationale comment for import GeometricBase: integrate
(src/GeometricMachineLearning.jl:16-19): a bare using supplies GeometricBase's exported
integrate, and it is the method definitions that would start a generic of this module's own. Pass
2 found the changelog gap that pass 1 had left, and three further text claims; commit 47f534bc
answers them in CHANGELOG.md:76, :78 and :118, each re-verified against
src/data_loader/data_loader.jl:425-427, src/map_to_cpu.jl:22-23 and the timing file of the
jet.jl run.

Unresolved: one nit. test/integration/qp_dispatch.jl:95's @test_throws MethodError poisson_matrix(4) * (a = [1.0, 2.0], b = [3.0, 4.0]) now tests nothing this package controls — a
Matrix times a NamedTuple is a MethodError either way. The part's spec decides that the row
keeps its subject, so it stays; it is listed under Recorded, not fixed below.

Not checked by the verifier: points 2–5 were not re-measured (no executable change after pass
1); the suite, the docs build and its doctests, JET standalone and the metal group were not run
there. The gates above are this part's runs.

Critic

Verdict: PASS. All seven earlier blocking defects are fixed, the fix diff adds none, and all nine mutants are caught.

clause domain on origin/main on the branch verdict
Test files the fix diff reaches accuracy.jl, batch.jl, map_to_cpu.jl, quality/aqua.jl, exports.jl, docs_make_checks_references.jl base column from base_facts.jl accuracy 37/37, batch 314/314, map_to_cpu 51/51, aqua 17/17, exports 19/19, docs_make_checks_references 6/6, together 444/444 PASS
every §3 scenario whose check is head_tree.sh the head tree n/a head_tree.sh: PASS PASS
every §3 scenario whose check is head_facts.jl the head tree base_facts.jl: "accuracy of a Float32 loader is a Float64", "copy keeps the JLBackend backend", the halved error count 40 ok, PASS, including the mixed pair Float32, the halved 0.9166667, the static host backend CPU(true) PASS
accuracy: halved, not floored; type is T; the mixed pair (Float32, Float64), mixed Float64 model on a Float32 loader — MU4 and 1b's OWN-c, OWN-floor and OWN-unhalved all CAUGHT, 4/4 PASS
map_to_cpu: a host copy keeps the host backend it was handed, CPU(true) included CPU(), CPU(true), JLBackend × both precisions — MU3, MU11, OWN-host and OWN-static all CAUGHT, 4/4 PASS
the batch testset forwards the window, in both precisions with eltype (Float32, Float64) — MU6 CAUGHT PASS
edges of the fix code 3 backends × 2 precisions; accuracy with n ∈ {1, 7}, Int64 and Int32 labels, every sample unlabelled, a JLArray loader under allowscalar(false) — @inferred map_to_cpu(nn) passes for all six cases; unlabelled gives 0.5 of T; a JLArray loader answers PASS
docs/src/index.md: two testsets, both named, no PoissonTensor metal.jl:35, :54 — "holds 2 @testset(s); index.md says two", exit 0 PASS
house rule: comments in the present tense the fix diff's + lines — no hit, exit 0 PASS

Earlier blocking defects, all fixed: the tutorial's solve! in its @example block (prev/ gives
UndefVarError in Main.__atexample__named__toda_lattice, tree/ passes); index.md's "three
testsets"; the spec sentence naming tensor_cayley; the Batch and map_to_cpu testsets' precision
loops and allowscalar(false); the three history comments; the accuracy testset that could not catch
a wrong element type.

Known issues added

None. The branch edits two existing entries rather than adding one: K17 loses its PoissonTensor * v
half with the type, and K3's heading and last sentence lose the Poisson-tensor kernel.

Recorded, not fixed

  • The pins.jl harness gap. pins.jl replaces a branch copy's src/ and ext/ with the base's
    and leaves the copy's Project.toml. The base's src/GeometricMachineLearning.jl has
    using InteractiveUtils and using TimerOutputs, and this branch removes both from [deps], so a
    copy of this branch's tree cannot load the base's source: no unit of this branch can be pinned on
    the base with pins.jl as it stands.
  • poisson_matrix is written three times — in test/helpers/poisson.jl,
    scripts/utilities/vector_fields.jl and scripts/verification/sympnet_upscaling_symplecticity.jl
    — because each script stands alone and includes no test helper. A shared copy would need a
    src/ home that neither a test nor a script should reach.
  • test/integration/qp_dispatch.jl's MethodError row is now vacuous: a Matrix times a
    NamedTuple has no method with or without this change. The row keeps its subject, which is the
    part's recorded decision; a stronger refusal would assert _vcat on other keys.
  • Pre-existing, not this part's: the HRedSys docstring's "standard constructor" block
    (src/reduced_system/reduced_system.jl:13-15) documents a call that matches no method — keywords
    where the arguments are positional, h_full missing, integrator absent, and projection_error
    a function of an HRedSys rather than an argument. That is the remaining half of HRedSys standard constructor is unusable: parameters keyword defaults to itself #293.
  • Pre-existing, not this part's: KNOWN_ISSUES.md K3's evidence names
    augment_zeros! in src/data_loader/tensor_assign.jl and skew_mat_mul_k! in
    src/kernels/mat_tensor_mul.jl, and the release removed both in part M3 (Products through GEMM: one _mul, slicing assign_q_and_p, zero and other-precision seeds in the kernel rrules (part M3) #345).

Decisions taken without the user

Decided (advisor, 2026-10-26, medium): the REMOVED requirement on PoissonTensor reads "The name SHALL be gone from src/, docs/src/, scripts/ and KNOWN_ISSUES.md, and from every file of test/ except test/quality/exports.jl, whose not-exported gate names it to assert that the package neither exports nor defines it." The scenarios the name is gone and the files that carry the name take the same exception, and head_tree.sh counts the four directories and adds that grep -rlF PoissonTensor test names exactly test/quality/exports.jl. The promise the check encodes — that MU10 is killed by a test in the repository — is what the exception protects.

Decided (advisor, 2026-10-26, high): GML keeps import GeometricOptimizers: update!, although no line of src/ uses it, because without it GeometricMachineLearning.update! resolves through using ProgressMeter to ProgressMeter.update!. test/quality/aqua.jl exempts that one name from no_stale_explicit_imports and gives this reason, and GML defines no method on update!. The three comments that say GML adds methods to update! are corrected: src/optimizers/optimizer.jl:4-5, test/quality/exports.jl:25-26 and §2 Seams of this spec.

Clauses the head does not meet as the spec writes it: none. spec-validate.py --head passes every
check: of the delta at 47f534bc.

Nine items of section M6, each of them a defect a user meets:

- `HRedSys` takes its integrator from the caller: all three constructors require
  the keyword, and the positional one defaults `parameters` to `NullParameters()`.
  The base defaulted both to undefined names, so a plain call raised `UndefVarError`.
- `map_to_cpu` puts the copy on the CPU, in its backend and in its fourth type
  parameter, where it kept the device backend it was copied off.
- `accuracy` returns the fraction in the element type of the loader's input.
- The seven trajectory methods move off `Base.iterate` onto
  `GeometricBase.integrate`, which this package imports and exports, with the 74
  call sites in 27 files, the two `@docs` entries and the two `@ref`s. The two
  sequence-length ternaries become one `_seq_length(arch, ics)`.
- `PoissonTensor` and its `GPUArraysCore` extension are removed; a site that used
  the tensor builds `[0 I; -I 0]`, and one that used the 2-form writes it out.
- `update!`, `solve!` and `iterate` leave the exports, `Linear` and
  `NeuralNetwork` are exported once each, and `test/quality/exports.jl` gains the
  four names that must not be exported.
- `InteractiveUtils` and `TimerOutputs` leave `[deps]` and `[compat]`.

`KNOWN_ISSUES.md` K17 and K3 lose the sentences about the removed type and kernel.
The extension's three products are the removed PoissonTensor's, so the file goes
with the type. Its test file moves to test/integration/triangular.jl, which holds
the same assertions on the structured matrix types this package re-exports: with
src/arrays/ gone there is no directory for it to mirror.
`src/optimizers/optimizer.jl` said that `update!` is GeometricOptimizers'
verb and that this package adds methods to it. It defines none: the module file
imports the name so that `GeometricMachineLearning.update!` is
GeometricBase's rather than `ProgressMeter`'s, which `using ProgressMeter`
would otherwise supply.

`test/quality/exports.jl` said the same of all three bound names in its
not-exported list. Only `solve!` carries this package's methods there; `update!`
and `iterate` are their owners' objects.
…view found

The error count of `accuracy` is halved rather than floored, so a sample that carries no
label counts half an error: a `Float32` loader with one unlabelled sample gives
`0.9166667` where a floored count gives `1.0`. The fraction is read in the loader's element
type, which the `Float64` model on a `Float32` loader pins, and both the unlabelled case
and that mixed pair are tests. The two testsets the part adds loop over `Float32` and
`Float64` and assert the element type of each result.

`map_to_cpu` keeps the host backend it was handed, so `CPU(; static = true)` comes back
static and a device network lands on `CPU()`. Its device testset runs under
`allowscalar(false)`.

The tutorial that solves a PSD network imports `solve!` in its own `@example` block, which
is the module Documenter evaluates it in. `README.md` calls `integrate`, the device file's
sentence in `docs/src/index.md` says it runs two testsets, and the Metal label names what
that file runs.

The comments that narrated a change say what the code does instead, the `HRedSys` docstring
states which keyword is required and what `parameters` defaults to, and the module's
dangling arrays heading is gone. The two scripts that build the canonical matrix carry the
guard the test helper has.
@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 5 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 07285c48-b6aa-4c34-b8fb-01f6ebee7b61
Commits

Reviewing files that changed from the base of the PR and between 47f534b and 18877fa.

Files selected for processing: 12
  • CHANGELOG.md
  • KNOWN_ISSUES.md
  • scripts/utilities/vector_fields.jl
  • src/GeometricMachineLearning.jl
  • src/architectures/standard_transformer_integrator.jl
  • src/architectures/symplectic_transformer.jl
  • src/architectures/transformer_integrator.jl
  • src/data_loader/data_loader.jl
  • src/reduced_system/reduced_system.jl
  • test/Project.toml
  • test/data_loader/accuracy.jl
  • test/runtests.jl
📝 Summary

Summary by CodeRabbit

  • Breaking Changes

    • Neural-network trajectory generation now uses integrate instead of iterate.
    • PoissonTensor and its GPU multiplication support have been removed.
    • HRedSys constructors now require an explicit integrator.
    • update! and solve! are no longer exported; import them from their defining packages when needed.
  • Bug Fixes

    • Accuracy results now match the input data’s element type, and unlabeled samples are handled correctly.
    • Mapping networks to CPU now supports device-backed networks while preserving CPU backend settings.
  • Documentation

    • Updated examples and guides to reflect the trajectory API and current Poisson-matrix usage.

The pull request renames neural-network trajectory methods to integrate, removes PoissonTensor and its GPU extension, and updates accuracy, CPU mapping, and HRedSys constructor behavior. It also changes package exports, dependencies, tests, examples, and documentation.

Changes: Trajectory Integration API

Layer / File(s) Summary
Integration methods and dispatch
src/GeometricMachineLearning.jl, src/architectures/*, src/data_loader/batch.jl
Trajectory methods for neural networks and transformer batches now extend integrate instead of Base.iterate. Transformer sequence-length selection uses _seq_length.
Examples and trajectory callers
README.md, docs/src/architectures/*, docs/src/tutorials/*, scripts/reproduction/*, scripts/utilities/ensemble_plots.jl
Examples, tutorials, reproduction scripts, and plotting utilities call integrate with their previous prediction arguments.
Integration tests and notes
test/architectures/*integrator.jl, test/data_loader/batch.jl, test/integration/docstrings/layers_and_architectures.jl, CHANGELOG.md
Tests use integrate and cover sequence lengths, batch prediction windows, and trajectory pairings. The changelog describes the renamed method.

PoissonTensor Removal

Layer / File(s) Summary
Remove PoissonTensor and GPU support
src/arrays/poisson_tensor.jl, ext/GPUArraysCoreExt.jl, src/GeometricMachineLearning.jl, docs/src/*, KNOWN_ISSUES.md
The package removes PoissonTensor, its GPU overloads, and related documentation and known-issue claims.
Dense matrix replacements
scripts/utilities/vector_fields.jl, scripts/verification/sympnet_upscaling_symplecticity.jl, test/helpers/poisson.jl, test/architectures/*, test/integration/*, test/layers/sympnet_upscaling.jl
Scripts and tests use dense canonical Poisson matrices or direct formulas in place of PoissonTensor.
Poisson-related test updates
test/arrays/poisson_tensor.jl, test/devices/metal.jl, test/quality/*, test/runtests.jl
PoissonTensor-specific tests are removed, and related test registration and quality-check notes are updated.

Accuracy Result Type

Layer / File(s) Summary
Accuracy calculation and tests
src/data_loader/data_loader.jl, test/data_loader/accuracy.jl
accuracy converts its result to the data loader input type. Tests cover Float32 and Float64 loaders, mixed model and loader types, and unlabeled samples.

CPU Backend Mapping

Layer / File(s) Summary
Backend selection and tests
src/map_to_cpu.jl, test/map_to_cpu.jl
map_to_cpu preserves CPU backends and selects CPU() for non-CPU backends. Tests check the returned backend, parameters, element types, and architecture.

HRedSys Integrator Requirement

Layer / File(s) Summary
Constructor keywords and tests
src/reduced_system/reduced_system.jl, test/reduced_system/reduced_system.jl
Each HRedSys constructor requires an explicit integrator. The standard constructor defaults parameters to NullParameters(), and tests cover these keyword requirements.

Package API and Dependency Updates

Layer / File(s) Summary
Exports and dependency declarations
src/GeometricMachineLearning.jl, src/optimizers/optimizer.jl, Project.toml, test/quality/exports.jl, test/quality/aqua.jl, CHANGELOG.md
The package adjusts exports and removes dependency and compatibility entries. Export tests check the bindings and names.
Explicit imports and documentation checks
docs/check_references.jl, docs/make.jl, docs/src/tutorials/symplectic_autoencoder.md, scripts/reproduction/symplectic_autoencoders/*, test/architectures/*, test/data_loader/data_loader.jl, CHANGELOG.md
Documentation, scripts, and tests import solve! explicitly where needed.

Priority: Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Possibly related PRs

Merge Risk: Moderate · up to 47f53

The vector-field scripts cannot produce trajectories with their current matrix dimensions, and some accepted accuracy inputs fail. Fix those paths before merging; the documentation examples also need correction.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title "Part M6: small API corrections for GeometricMachineLearning 0.9" is specific to this PR's changes. The changeset removes PoissonTensor, switches trajectory methods from Base.iterate to Geom…
Description check Passed The description explains the PR's purpose and lists its concrete changes: the PoissonTensor removal, the iterate-to-integrate rename, the export changes, the HRedSys integrator requirement, and the ac…
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "Part M6: small API corrections for GeometricMachineLearning 0.9" is specific to this PR's changes. The changeset removes PoissonTensor, switches trajectory methods from Base.iterate to GeometricBase.integrate, stops exporting update! and solve!, requires integrator in HRedSys, and fixes accuracy and map_to_cpu. The title does not name the main changes, so it is only partially related. It also uses the term 'Part M6', which a teammate scanning history would not understand without context.

Full details: Description check

Explanation

The description explains the PR's purpose and lists its concrete changes: the PoissonTensor removal, the iterate-to-integrate rename, the export changes, the HRedSys integrator requirement, and the accuracy and map_to_cpu fixes. It also covers validation runs and the CHANGELOG, tutorial, and script updates. It is clearly related to the changeset.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (60 skipped: 60 unsupported.)

Finishing Touches: 💡 1
⚔️ Resolve merge conflicts: 💡
  • Resolve merge conflict in branch simplify/part-m6
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

Minor · Add integrator to both documented constructor forms. · reduced_system.jl:13

src/reduced_system/reduced_system.jl:13
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add integrator to both documented constructor forms.

Both call forms omit the newly required keyword. A caller who follows either form gets UndefKeywordError. Show integrator = ... in both forms. Also show encoder and decoder as positional arguments in the standard form, as the constructor requires.

Also applies to: 31-31


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 99e3cd2a-7b57-4b28-bb8b-9967d76c07c1
Commits

Reviewing files that changed from the base of the PR and between d7a488c and 47f534b.

Files selected for processing: 67
  • CHANGELOG.md
  • KNOWN_ISSUES.md
  • Project.toml
  • README.md
  • docs/check_references.jl
  • docs/make.jl
  • docs/src/architectures/neural_network_integrators.md
  • docs/src/index.md
  • docs/src/structure_preservation/symplecticity.md
  • docs/src/tutorials/linear_symplectic_transformer.md
  • docs/src/tutorials/matrix_softmax.md
  • docs/src/tutorials/softmax_comparison.md
  • docs/src/tutorials/symplectic_autoencoder.md
  • docs/src/tutorials/symplectic_transformer.md
  • docs/src/tutorials/sympnet_tutorial.md
  • docs/src/tutorials/volume_preserving_attention.md
  • docs/src/tutorials/volume_preserving_transformer_rigid_body.md
  • ext/GPUArraysCoreExt.jl
  • scripts/reproduction/linear_symplectic_transformer_gpu.jl
  • scripts/reproduction/symplectic_autoencoders/online_sympnet.jl
  • scripts/reproduction/symplectic_autoencoders/training.jl
  • scripts/reproduction/symplectic_transformer/double_pendulum.jl
  • scripts/reproduction/symplectic_transformer/double_pendulum_short_integration.jl
  • scripts/reproduction/sympnets/sympnet_toda_lattice.jl
  • scripts/reproduction/transformer_integrator/softmax_comparison.jl
  • scripts/reproduction/transformer_integrator/symplectic_transformer.jl
  • scripts/reproduction/transformer_integrator/symplectic_transformer_vector_softmax.jl
  • scripts/reproduction/volume_preserving_feedforward/abc_flow.jl
  • scripts/reproduction/volume_preserving_feedforward/double_pendulum.jl
  • scripts/reproduction/volume_preserving_feedforward/rigid_body.jl
  • scripts/reproduction/volume_preserving_transformer/rigid_body.jl
  • scripts/utilities/ensemble_plots.jl
  • scripts/utilities/vector_fields.jl
  • scripts/verification/sympnet_upscaling_symplecticity.jl
  • src/GeometricMachineLearning.jl
  • src/architectures/neural_network_integrator.jl
  • src/architectures/resnet.jl
  • src/architectures/transformer_integrator.jl
  • src/arrays/poisson_tensor.jl
  • src/data_loader/batch.jl
  • src/data_loader/data_loader.jl
  • src/map_to_cpu.jl
  • src/optimizers/optimizer.jl
  • src/reduced_system/reduced_system.jl
  • src/utils.jl
  • test/architectures/psd_architecture_tests.jl
  • test/architectures/sae_error_lower_than_psd_error.jl
  • test/architectures/standard_transformer_integrator.jl
  • test/architectures/symplectic_autoencoder_tests.jl
  • test/architectures/sympnet_integrator.jl
  • test/arrays/poisson_tensor.jl
  • test/data_loader/accuracy.jl
  • test/data_loader/batch.jl
  • test/data_loader/data_loader.jl
  • test/devices/metal.jl
  • test/helpers/poisson.jl
  • test/integration/docstrings/layers_and_architectures.jl
  • test/integration/docstrings/utilities.jl
  • test/integration/qp_dispatch.jl
  • test/integration/triangular.jl
  • test/layers/sympnet_upscaling.jl
  • test/map_to_cpu.jl
  • test/quality/aqua.jl
  • test/quality/exports.jl
  • test/quality/jet.jl
  • test/reduced_system/reduced_system.jl
  • test/runtests.jl
Files with no reviewable changes: 6
  • test/devices/metal.jl
  • Project.toml
  • docs/src/structure_preservation/symplecticity.md
  • ext/GPUArraysCoreExt.jl
  • test/arrays/poisson_tensor.jl
  • src/arrays/poisson_tensor.jl

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Comment thread CHANGELOG.md Resolved
Comment thread scripts/utilities/vector_fields.jl Resolved
Comment thread src/data_loader/data_loader.jl Resolved
Comment thread src/data_loader/data_loader.jl Resolved
Comment thread src/data_loader/data_loader.jl Resolved
@codecov

codecov Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.07%. Comparing base (d7a488c) to head (18877fa).

Files with missing lines Patch % Lines
src/architectures/transformer_integrator.jl 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #358      +/-   ##
==========================================
+ Coverage   92.32%   93.07%   +0.75%     
==========================================
  Files          53       51       -2     
  Lines        1186     1156      -30     
==========================================
- Hits         1095     1076      -19     
+ Misses         91       80      -11     

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

@michakraus michakraus left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JuliaGNI/GeometricMachineLearning.jl PR #358 — Part M6: small API corrections for GeometricMachineLearning 0.9

Verdict: request changes. It is posted as a comment because GitHub does not let the author's own account request changes on its own PR.
Reviewed: 8d48786258a35eeffb452128056b6414c7357d53

CI on this head, read job by job (run 40335239943):

  • Julia pre fails before any test runs. The log says Failed to precompile GPUCompiler [61eb1bfa-…] to ".../compiled/v1.14/GPUCompiler/…", after UndefVarError: libLLVMExtra not defined in LLVM.API. This is a dependency failure on 1.14.0-alpha1. main at d7a488c2 (run 40310215232, job 121529219640) fails on the same line, so the diff does not cause it.
  • Julia nightly has the same failure on 1.15.0-DEV.143: Failed to precompile GPUCompiler …, and Metal (skipped, GPUCompiler failed to precompile). main at d7a488c2 (job 121529219595) fails the same way. The diff does not cause it.
  • Downgrade fails at the forcedeps check, which is a resolver failure and not a test failure: forcedeps check failed: Zygote resolved to 0.7.5 but lower bound is 0.7.0, plus the same for ChainRulesCore 1.25.1/1.0.0, NNlib 0.9.30/0.8.0 and KernelAbstractions 0.9.38/0.9.0. main (job 121529219128) fails on the same four packages and on a fifth, GPUArraysCore resolved to 0.2.1 but lower bound is 0.2.0. This PR removes the fifth. The diff does not change any of the four floors that remain.
  • codecov/patch: 86.36% of diff hit (target 92.32%). codecov/project: 93.07% (+0.75%). I did not find the uncovered lines (see Not checked).
  • Every other job passes: Julia 1 and Julia min on ubuntu, macOS and windows, Doctests, Documentation, Metal min and 1, and Scripts.

Findings

# severity file:line claim evidence
1 bug src/data_loader/data_loader.jl:427 T(n - errors) / T(n) raises InexactError on a loader whose input is an integer type when the error count is not whole. That is the case for the unlabelled sample that this PR pins as "half an error". The base formula (n - errors/T1(2)) / n returned a Float64 in that case. When the count is whole, the result is still a Float64 and not T, which contradicts the new docstring sentence. Scratch environment with the PR head developed, Julia 1.13.2. DataLoader(rand(0:3, 4, 1, 6), out) with one all-zero target column gives InexactError: Int64(1.5). The same loader with every column labelled gives (0.3333333333333333, Float64). A fix that keeps the Float32/Float64 behaviour and covers integer inputs is F = float(T); F(n - errors) / n. It gives 0.75 Float64 for Int and a Float32 for Float32.
2 quality src/data_loader/data_loader.jl:407-408 The new docstring says the result is "the fraction of correctly labelled samples". The new testset test/data_loader/accuracy.jl:93 pins one unlabelled sample out of six with five correct as (6 - 1/2)/6 = 0.9167, but the fraction of correctly labelled samples is 5/6. Either the docstring states the half-error rule, or the unlabelled case gets a defined meaning (excluded, or counted as wrong). Pinning the formula's by-product in a test makes it look like a contract. The docstring text at :407-408 against the @test a == (T(n_params) - T(1) / 2) / T(n_params) at test/data_loader/accuracy.jl:93-100.
3 quality src/reduced_system/reduced_system.jl:13, :31 Neither documented call form works. :31 shows HRedSys(odeensemble, encoder, decoder), but this PR makes integrator required, so that call raises UndefKeywordError. :13 shows HRedSys(N, n; encoder, decoder, v_full, f_full, v_reduced, f_reduced, parameters, timespan, timestep, ics, projection_error), which matches no method: encoder through ics are positional, h_full is missing, integrator is missing, and projection_error is not an argument. Only the prose at :22 and :34 names the keyword. The PR body calls :13 pre-existing, but this PR edits the same docstring, so fixing :13 belongs in this change. This confirms the outside-diff CodeRabbit comment on 47f534b, which still holds at this head. methods(HRedSys) at this head lists HRedSys(N::Integer, n::Integer, encoder, decoder, v_full, f_full, h_full, timespan::Tuple, timestep::T, ics; parameters, v_reduced, f_reduced, h_reduced, integrator) and two (problem, encoder, decoder; integrator) methods. test/reduced_system/reduced_system.jl:151-152 asserts that the :31 form raises UndefKeywordError, and it passes. Suggested signature lines: HRedSys(N, n, encoder, decoder, v_full, f_full, h_full, timespan, timestep, ics; integrator, parameters = NullParameters(), v_reduced, f_reduced, h_reduced) and HRedSys(odeensemble, encoder, decoder; integrator). Also, :23 calls parameters "a NamedTuple" but gives it the default NullParameters().
4 quality scripts/utilities/vector_fields.jl:34-50 The PR rewrites v_field and v_field_explicit, but nothing calls them. They also have a pre-existing dimension mismatch: full_mat is 2m × 2m, while poisson_matrix(size(K, 1)) is m × m. This applied to PoissonTensor(size(K, 1)) on main too. Delete the two functions instead of porting them. That also removes one of the three copies of poisson_matrix. grep -rn -E "v_field(_explicit)?\b" over the repository finds only the two definitions. K = assemble_matrix(...).parent is (Ñ+2) × (Ñ+2), and full_mat = hcat(vcat(K + K', zero(K)), vcat(zero(K), one(K)*Δx)).
5 nit CHANGELOG.md:20 The migration recipe [0 I(n); -I(n) 0] raises an error for every n > 1, because a scalar 0 is not an n × n block. The PR body uses the same text. The comments in test/helpers/poisson.jl, scripts/utilities/vector_fields.jl and scripts/verification/sympnet_upscaling_symplecticity.jl describe the matrix the same way, but their code is correct. [0 I(n); -I(n) 0] with n = 2 gives DimensionMismatch: incompatible shape in element 2. [zeros(n, n) I; -I zeros(n, n)] gives the 4×4 matrix and is equal to the helper's result.
6 nit src/data_loader/data_loader.jl:426 / T1(2) raises InexactError: Bool(2) for Bool one-hot labels, which the signature T1 <: Integer accepts. The base has the same T1(2), so this PR did not introduce it. The PR rewrites this line, though, and / 2 fixes it. accuracy(model, ps, DataLoader(rand(4, 1, 6), Bool.(out))) gives InexactError: Bool(2).
7 nit KNOWN_ISSUES.md:219-224 The PR edits K3's heading and its last sentence. The evidence in between is still stale: "142 pass, 3 broken" (the CHANGELOG now says 50 assertions, none broken), augment_zeros! in src/data_loader/tensor_assign.jl, and skew_mat_mul_k! in src/kernels/mat_tensor_mul.jl. The PR body lists this as pre-existing, but it sits in the paragraph this PR rewrites. grep -rn -E "skew_mat_mul_k!|augment_zeros!" src finds no match. src/kernels/ has no mat_tensor_mul.jl.
8 nit test/Project.toml:9 GPUArraysCore is now a test-only dependency (test/map_to_cpu.jl, test/devices/jlarray.jl, test/layers/positional_encoding.jl and test/layers/volume_preserving_attention.jl use it). It lost the "0.2" bound that the root [weakdeps]/[compat] gave it, and test/Project.toml adds no bound of its own. git diff origin/main... removes GPUArraysCore = "0.2" from Project.toml. test/Project.toml is unchanged and has no GPUArraysCore entry in [compat].
9 nit src/architectures/transformer_integrator.jl:18-23 _seq_length branches on isa over two concrete types. Two methods say the same with dispatch: _seq_length(arch::TransformerIntegrator, ::AbstractMatrix) = arch.seq_length and _seq_length(::Union{StandardTransformerIntegrator, SymplecticTransformer}, ics::AbstractMatrix) = size(ics, 2). The second method goes after both types are defined. Optional: the current version folds to a constant. code_typed(_seq_length, (VolumePreservingTransformer{…}, Matrix{Float32})) is a single getfield(arch, :seq_length)::Int64, so the branch costs nothing at run time. This is about readability only.
10 nit GeometricOptimizers/src/manifolds/symplectic_stiefel_manifold.jl:70-73 (other repository) After this merges, that comment is wrong: it says GeometricMachineLearning "exports a PoissonTensor type" and that "unifying the two is a separate change". It needs a follow-up there. Read at that path.

Verified and fine

  • I ran 10 test files from this head in a scratch environment (Julia 1.13.2, GeometricOptimizers 0.9.1, GeometricBase 0.15.2, GeometricIntegrators 0.18.7, ExplicitImports 1.15.0, Aqua 0.8.18): reduced_system/reduced_system.jl, map_to_cpu.jl, data_loader/accuracy.jl, data_loader/batch.jl, quality/exports.jl, quality/aqua.jl (with test_explicit_imports), integration/qp_dispatch.jl, integration/docstrings/layers_and_architectures.jl, architectures/standard_transformer_integrator.jl and architectures/sympnet_integrator.jl. Result: 538 pass, 0 fail, 0 error.
  • GeometricMachineLearning.integrate === GeometricIntegrators.integrate === GeometricBase.integrate is true. All seven trajectory methods are on that generic, with a NeuralNetwork first argument, so this is not piracy. integrate, Linear and NeuralNetwork are exported. update! and solve! are not.
  • The claim that the update! import is needed holds. GeometricBase does not export update! and ProgressMeter does. In a module that only does using GeometricBase; using ProgressMeter, update! resolves to ProgressMeter.
  • map_to_cpu: nn.backend isa CPU ? nn.backend : CPU() is decided from the type parameter, and the JLArray and CPU(; static = true) testsets pass.
  • _seq_length infers Int64 for StandardTransformerIntegrator, VolumePreservingTransformer and LinearSymplecticTransformer, and standard_transformer_integrator.jl asserts the stored-length branch directly.
  • In src/, docs/src/, scripts/ and test/, no iterate(nn…, PoissonTensor or ImplicitMidpoint() default is left. The remaining names are the NOT_EXPORTED gate and history entries in the CHANGELOG. Every HRedSys( call site in docs/, scripts/ and test/ passes integrator.
  • JuliaFormatter (sciml) reports every changed file under src/ and test/, plus docs/make.jl, docs/check_references.jl and the two changed utility/verification scripts, as formatted.
  • The PR body states that the pre, nightly and Downgrade jobs are red on main as well. That is true; see CI above. The PR body has no closing keyword, and closingIssuesReferences is empty.

Not checked

  • Which lines codecov/patch counts as missed. The Codecov API was not reachable from here, and I did not run a local coverage build of the suite.
  • The metal group, the docs build and the doctests. CI's Metal, Documentation and Doctests jobs pass on this head, and I did not run them again.
  • The full suite. I ran only the 10 files listed above.

accuracy raised InexactError for an integer input when a sample carried no label (half an error), and for Bool labels on the halving; it now computes in float(T). A new testset covers the integer input. The HRedSys docstring shows integrator in both call forms and the standard form matches the positional method. The two Unreleased changelog bullets are corrected to match: the accuracy formula, and a Poisson-tensor recipe that works for n > 1.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@michakraus

Copy link
Copy Markdown
Member Author

Resolution of the findings in the review above (#358 (review)), as of e59e9ae.

# severity finding action
1 bug accuracy: T(n - errors) / T(n) raises InexactError for an integer input with a half error, and returns Float64 (not T) for a whole count fixed in e59e9ae: the fraction is computed in F = float(T). Before the fix, an Int64 input with one unlabelled sample of six raised InexactError: Int64(5.5). After it: 0.9166666666666666 == 5.5/6, a Float64; a fully labelled integer loader gives 1.0. New testset "an integer input gives a Float64, half errors included" in test/data_loader/accuracy.jl.
2 quality the accuracy docstring says "fraction of correctly labelled samples", but an unlabelled sample counts as half an error fixed in e59e9ae: the docstring now says one minus the fraction of errors, with a wrong label as one error and no label as half an error, and names the result type float(T).
3 quality HRedSys docstring: both call forms omit the required integrator; the standard form matches no method; parameters is called "a NamedTuple" fixed in e59e9ae: the standard form is now HRedSys(N, n, encoder, decoder, v_full, f_full, h_full, timespan, timestep, ics; integrator, parameters, v_reduced, f_reduced, h_reduced), the other form shows ; integrator, and parameters reads "a NamedTuple or NullParameters()". The bullet list still has no h_full / h_reduced entries; that gap is older than this PR and is left.
4 quality v_field / v_field_explicit in scripts/utilities/vector_fields.jl are not called and have a dimension mismatch (poisson_matrix(size(K, 1)) is m×m, full_mat 2m×2m) not fixed: the mismatch is the same on main with PoissonTensor, and deleting two script functions is the author's decision, not a review fix. Confirmed that nothing calls them; the file is included by scripts/reproduction/symplectic_autoencoders/training.jl and integration.jl.
5 nit CHANGELOG migration recipe [0 I(n); -I(n) 0] fails for n > 1 fixed in e59e9ae: the recipe is [zeros(n, n) I; -I zeros(n, n)]. Checked: the old form gives DimensionMismatch: incompatible shape in element 2 for n = 2; the new one gives the 4×4 canonical matrix.
6 nit / T1(2) raises InexactError: Bool(2) for Bool labels fixed in e59e9ae: the count is halved with / 2. A Float32 input with Bool labels and one unlabelled sample now gives 0.9166667f0.
7 nit KNOWN_ISSUES K3 evidence names augment_zeros!, skew_mat_mul_k! and src/kernels/mat_tensor_mul.jl, which no longer exist, and an old pass count not fixed: confirmed that none of the three names occurs in src/ or test/, but those evidence lines are not part of this diff, and a correct count needs a new mutation run of test/quality/jet.jl.
8 nit GPUArraysCore is test-only now and has no compat bound in test/Project.toml not fixed: test/Project.toml is outside this diff. A test-only dependency keeps its own bound, so the follow-up is a GPUArraysCore = "0.2" entry there.
9 nit _seq_length branches on isa; two methods would read better not fixed: optional, and it folds to a constant.
10 nit a comment in GeometricOptimizers (src/manifolds/symplectic_stiefel_manifold.jl) says this package exports PoissonTensor not fixed: another repository; it needs a follow-up there after this merges.

Checks run on e59e9ae before the push: test/data_loader/accuracy.jl and test/reduced_system/reduced_system.jl pass (59 of 59, Julia 1.13.2); pre-commit passed formatting, fatou lint, NFC and the load test.

CI on the previous head (run 40335239943): the review above traces the red "Julia pre", "Julia nightly" and "Downgrade" jobs to failures that main shows too, not to this diff. None of the three is a required check. codecov/patch was below target there; which lines it counts as missed was not checked.

_seq_length dispatches on the architecture instead of branching on isa: the generic method reads arch.seq_length, and StandardTransformerIntegrator and SymplecticTransformer each take the width of the input, beside their own definitions. scripts/utilities/vector_fields.jl loses v_field and v_field_explicit, which nothing called, with the poisson_matrix helper and the LinearAlgebra import that only they used. test/Project.toml bounds the now test-only GPUArraysCore at 0.2, the root bound it had on main. KNOWN_ISSUES K3 names the kernel launches that exist and gives the counts of a new mutation run of test/quality/jet.jl.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@michakraus

Copy link
Copy Markdown
Member Author

b53ebb4 resolves four of the five findings that the previous comment left open:

  • 4 fixed: scripts/utilities/vector_fields.jl no longer has v_field and v_field_explicit, or the poisson_matrix helper and using LinearAlgebra: I that only they used. The two scripts that include the file call v_f_hamiltonian, so the includes stay. A grep over scripts/, docs/, test/ and src/ finds no other reader.
  • 7 fixed: K3 in KNOWN_ISSUES.md names the kernel launches that exist now, with the counts of a new mutation run of test/quality/jet.jl (Julia 1.13.2). A barrier on a launch argument survives at three sites, each with 50 pass. A barrier in the launcher's @assert is caught (47 pass, 3 fail), and so is a barrier inside the body of tensor_tensor_mul_kernel!.
  • 8 fixed: test/Project.toml bounds GPUArraysCore = "0.2", the bound the root Project.toml had on main.
  • 9 fixed: _seq_length dispatches. The generic method reads arch.seq_length, and StandardTransformerIntegrator and SymplecticTransformer each have a method beside their definitions. The existing test of all three cases passes.
  • 10 open: the comment in GeometricOptimizers that says this package exports PoissonTensor is true until this PR merges and a release carries the removal, so the edit there waits.

Checks before the push: the transformer, docstring, Aqua and JET test files pass (93 of 93). pre-commit passed formatting, fatou lint, NFC and the load test. No changelog entry is needed.

The halving of the error count is in floating point, so a Bool target gives the value of the Int64 one; the new testset fails with InexactError: Bool(2) on the old divisor. The Unreleased accuracy bullet now says where the count is taken.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@michakraus

Copy link
Copy Markdown
Member Author

The CodeRabbit findings on this PR, and what happened to each (as of 18877fa):

CodeRabbit finding action
CHANGELOG: [0 I(n); -I(n) 0] fails for n > 1 (inline, line 20) fixed in e59e9ae (review finding 5)
vector_fields.jl: poisson_matrix(size(K, 1)) has the wrong dimension (inline, line 37) fixed in b53ebb4: v_field and v_field_explicit are removed, because nothing calls them (review finding 4)
accuracy docstring does not describe the half error of an unlabelled sample (inline, line 408) fixed in e59e9ae (review finding 2)
accuracy: / T1(2) raises InexactError for Bool labels, and a Boolean-target test is wanted (inline, line 426) the divisor was fixed in e59e9ae (review finding 6). 18877fa adds the test: "a Bool target gives the accuracy of the Int64 one". It fails with InexactError: Bool(2) when the old divisor is restored.
accuracy: an integer input raises InexactError on a half error (inline, line 427) fixed in e59e9ae (review finding 1)
HRedSys docstring omits integrator in both call forms (review body, outside the diff) fixed in e59e9ae (review finding 3)
merge-risk note: vector-field dimensions, accuracy inputs, documentation examples (summary comment) the same findings as the rows above, all fixed
"Resolve merge conflict in branch simplify/part-m6" (summary comment) not applicable: GitHub reports the PR as MERGEABLE at b53ebb4, so there is no conflict to resolve
Title check: "Part M6" is not self-explanatory (summary comment) not changed: the title is the author's choice, and the check itself passes

test/data_loader/accuracy.jl passes (45 of 45, Julia 1.13.2), and pre-commit passed formatting, fatou lint, NFC and the load test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

1 participant