Repository navigation
Remove the Hamiltonian and Lagrangian networks (now in their own packages) - #354
Merged
Merged
Conversation
The Hamiltonian networks, their losses and the symbolic pullback move to HamiltonianNeuralNetworks.jl; the Lagrangian network and its losses to LagrangianNeuralNetworks.jl. SymbolicNeuralNetworks and Symbolics leave [deps] and [compat]. The mixed test files keep their GML assertions. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Only the symbolic HNN pullback used parameter_eltype. The implicit-import and non-public name counts of aqua.jl and known issue C16 follow the removal. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…N and LNN removal docs/src/index.md no longer cites the Hamiltonian neural network as a GML architecture. The CHANGELOG states that C20 and K21 leave this package's KNOWN_ISSUES.md and that neither new package records them yet. The C-entry gives 4 non-public qualified accesses, as measured with ExplicitImports on this branch. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ader comment true `∇H`, `dH` and `get_data_set` of scripts/utilities/pendulum.jl lost their only callers with hnn_pendulum.jl and hnn_pendulum_simulation.jl. The JET check of an input-and-output `:TimeSeries` loader cited suite files that build `:RegularData`; the comment now says no suite loader has that type. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Warning Review limit reachedYou'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 21 minutes. View limit details
📝 Summary
Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly states that the Hamiltonian and Lagrangian networks are removed because they now live in separate packages. |
| Description check | |
The description directly explains the code removal, package migration, dependency updates, documentation changes, tests, and migration guidance. |
| Docstring Coverage | |
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 | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
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. (16 skipped: 16 unsupported.)
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- 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.
Comment @coderabbitai help to get the list of available commands.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #354 +/- ##
==========================================
- Coverage 91.61% 91.45% -0.16%
==========================================
Files 58 51 -7
Lines 1228 1112 -116
==========================================
- Hits 1125 1017 -108
+ Misses 103 95 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- 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:
9a8b2113-0763-4d49-a375-4b1b2168957e
-
CHANGELOG.mdKNOWN_ISSUES.mdProject.tomlREADME.mddocs/make.jldocs/src/architectures/hamiltonian_neural_network.mddocs/src/architectures/lagrangian_neural_network.mddocs/src/architectures/sympnet.mddocs/src/index.mddocs/src/structure_preservation/structure_preserving_neural_networks.mddocs/src/tutorials/hamiltonian_neural_network.mdext/GPUArraysCoreExt.jlscripts/reproduction/hnn_pendulum.jlscripts/reproduction/hnn_pendulum_simulation.jlscripts/reproduction/lnn_pendulum.jlscripts/runscripts.jlscripts/utilities/data_problem.jlscripts/utilities/pendulum.jlscripts/utilities/plots.jlscripts/verification/symplectic_euler_loss_modified_hamiltonian.jlsrc/GeometricMachineLearning.jlsrc/architectures/hamiltonian_neural_network.jlsrc/architectures/lagrangian_neural_network.jlsrc/arrays/poisson_tensor.jlsrc/data_loader/train.jlsrc/loss/hnn_loss.jlsrc/loss/lnn_loss.jlsrc/loss/symplectic_euler_loss.jlsrc/loss/variational_midpoint_loss.jlsrc/pullbacks/symbolic_hnn_pullback.jltest/architectures/forward_inference.jltest/architectures/hamiltonian_neural_network_tests.jltest/architectures/lagrangian_neural_network_tests.jltest/data_loader/train.jltest/integration/train_manual_architectures.jltest/loss/losses.jltest/loss/training_method_losses.jltest/pullbacks/symbolic_hnn_pullback.jltest/quality/aqua.jltest/quality/jet.jltest/runtests.jl
-
- test/pullbacks/symbolic_hnn_pullback.jl
- scripts/reproduction/lnn_pendulum.jl
- test/runtests.jl
- Project.toml
- src/loss/variational_midpoint_loss.jl
- test/loss/training_method_losses.jl
- docs/src/tutorials/hamiltonian_neural_network.md
- docs/src/architectures/hamiltonian_neural_network.md
- docs/make.jl
- docs/src/architectures/lagrangian_neural_network.md
- test/architectures/hamiltonian_neural_network_tests.jl
- scripts/utilities/data_problem.jl
- scripts/verification/symplectic_euler_loss_modified_hamiltonian.jl
- src/pullbacks/symbolic_hnn_pullback.jl
- test/integration/train_manual_architectures.jl
- scripts/reproduction/hnn_pendulum_simulation.jl
- test/architectures/lagrangian_neural_network_tests.jl
- scripts/utilities/pendulum.jl
- scripts/reproduction/hnn_pendulum.jl
- scripts/utilities/plots.jl
- src/architectures/lagrangian_neural_network.jl
- src/loss/symplectic_euler_loss.jl
- src/loss/hnn_loss.jl
- src/loss/lnn_loss.jl
- src/architectures/hamiltonian_neural_network.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.
-
michakraus
commented
Oct 24, 2026
michakraus
left a comment
michakraus
left a comment
Member
Author
There was a problem hiding this comment.
JuliaGNI/GeometricMachineLearning.jl PR #354 — Remove the Hamiltonian and Lagrangian networks (now in their own packages)
Verdict: request changes (posted as a comment review)
Reviewed: 0b037bcac653bba3293360c8c866009c5f766cc8 (the merge of main into e/remove-hnn-lnn; bc7a86d7 plus main at 6d8218aa)
CI, read per job (run 40089301217 and siblings, at the time of this review):
- Downgrade - ubuntu-latest: failed, and the failure is not caused by this PR. It is a resolver failure in
julia-downgrade-compat(forcedeps), not a test failure. GPUArraysCore (0.2.1 vs floor 0.2.0), Zygote (0.7.5 vs 0.7.0), ChainRulesCore (1.25.1 vs 1.0.0), NNlib (0.9.30 vs 0.8.0) and KernelAbstractions (0.9.38 vs 0.9.0) do not resolve to their floors.mainfails in the same way, with the same five packages, at2e7b7aca(job 120858148460, today) and ate759f206(run 39978614949). The removal of SymbolicNeuralNetworks and Symbolics only removes constraints. Two of the five come from GeometricOptimizers 0.9.0, whose[compat]hasChainRulesCore = "1.25.1"andKernelAbstractions = "0.9.38", against"1"and"0.9"here. I did not trace the other three. - Julia nightly - ubuntu-latest: failed, infrastructure.
GPUCompilerdoes not precompile on 1.15.0-DEV.115 (UndefVarError: libLLVMExtra not defined in LLVM.API). The nightly job ofmainfails too. - Doctests and Metal (Julia min): pass. Julia 1, Julia min (ubuntu, macOS, Windows), pre, Metal (Julia 1), Documentation and Scripts: still pending. This PR has no test signal from CI yet.
Findings
| # | severity | file:line | claim | evidence |
|---|---|---|---|---|
| 1 | quality | CHANGELOG.md:127 |
The [Unreleased] entry that raises the floor to julia = "1.12" gives the LNNLoss gradient overflow under SymbolicUtils as its reason. After this PR, that code is not in this package. The floor still holds, but for another reason: GeometricOptimizers 0.9 needs Julia 1.12. An unreleased entry must be true for its release. Restate the reason. |
GeometricOptimizers v0.9.0 Project.toml: julia = "1.12". The new bullet at CHANGELOG.md:20 covers "these types, losses or SymbolicPullback" and the SymbolicNeuralNetworks bound, not the reason for the floor. |
| 2 | quality | CHANGELOG.md:129, CHANGELOG.md:131 |
The same [Unreleased] section describes test/loss/training_method_losses.jl (moved into test/loss/, network shapes changed) and test/architectures/lagrangian_neural_network_tests.jl (moved from core to slow, 122.7 s). This PR deletes both files, so in 0.9.0 these sentences describe tests that do not exist. Drop those clauses. |
git diff --stat origin/main...origin/pr/354 shows both files deleted. A grep of lines 16–171 for deleted paths finds them at these two lines. |
| 3 | quality | test/loss/losses.jl:29-50 |
The PR removes the loop that checked "a bare NamedTuple reaches no method of a GML loss" for the four HNN/LNN losses. The testset now checks only TransformerLoss. AutoEncoderLoss has the same ps::NetworkParameters annotation (src/loss/losses.jl:151,156), and no test checks it. Add @test_throws MethodError AutoEncoderLoss()(model, params(ps), x) so that the stated property keeps more than one witness. |
Probe on head: AutoEncoderLoss()(m, ps, x) gives 1.35…. AutoEncoderLoss()(m, params(ps), x) gives MethodError. AutoEncoderLoss()(m, params(ps), x, x) gives ErrorException. git grep "test_throws MethodError" over test/ finds no loss case other than NetworkLoss(transformer, 1). |
| 4 | quality | test/quality/aqua.jl:78 |
:copyto! in NON_PUBLIC_QUALIFIED is dead. The PR edits this tuple, so remove the entry here rather than as a follow-up. |
ExplicitImports on head (all modules): 4 non-public qualified accesses, Initializer, dim (AbstractNeuralNetworks), jacobian (ForwardDiff), zeros (KernelAbstractions). There is no copyto!. The only copyto! in src/ext is Base.copyto! (src/data_loader/data_loader.jl:345). |
| 5 | quality | test/quality/jet.jl:130-136 |
The comment now says that no loader of the suite has the type DataLoader{T, A3{T}, A3{T}, :TimeSeries, Int}, and the line still checks only that type. After this PR, the suite builds input-and-output loaders with DataLoader(input, output), which gives :RegularData. No JET line checks that type with a float output. Retarget the line to the type the suite builds, or delete it. Do not keep a check of a signature that has no caller. The PR edits this comment, so the change belongs here. |
git grep ":TimeSeries" -- test: each :TimeSeries loader with an output has Array{Int, 3} output (test/data_loader/data_loader.jl:157, optimizer_functor_with_adam.jl:23-27, batch.jl:188). The :RegularData lines in jet.jl are at 118 (no output) and 156 (A3{Int} output). |
| 6 | quality | src/GeometricMachineLearning.jl:25-26, Project.toml:14,21,39,46 |
InteractiveUtils and TimerOutputs are dead dependencies. The using lines are the only uses, and they hide the dependencies from Aqua stale_deps. This is not caused by this PR (also true on main), but the PR removes two dependencies from the same using block and the same [deps] table, so the cleanup belongs in this change. |
A syntax-tree match of names(InteractiveUtils) and names(TimerOutputs) against Meta.parseall of every file under src/ and ext/: "no exported name used" for both. git grep "InteractiveUtils|TimerOutputs" origin/main -- src ext finds only the using lines. |
| 7 | quality | KNOWN_ISSUES.md (C20, K21 deleted); CHANGELOG.md:20 |
K21 (the field act of LagrangianNeuralNetwork) and C20 leave this file, and "neither new package records them yet". After the merge, a known defect is recorded in no KNOWN_ISSUES.md. Land K21 and C20 in LagrangianNeuralNetworks.jl and HamiltonianNeuralNetworks.jl before this PR merges. |
gh api …/HamiltonianNeuralNetworks.jl/contents/KNOWN_ISSUES.md has C7 and K15 only. The CHANGELOG bullet and the PR body ("Follow-ups") state that C20 and K21 have no record yet. |
| 8 | nit | src/data_loader/train.jl:21-22 |
From CodeRabbit, valid. The sentence names SymbolicPullback(arch) as the way that a HamiltonianNeuralNetworks.jl network trains. That pullback is built on HNNLoss alone (vector-field data), and SymplecticEulerLoss is the loss for consecutive states. Do not list the other package's losses here. Write "trains with a loss or a pullback of that package; see its manual", so that this docstring does not depend on that package's API. |
HamiltonianNeuralNetworks.jl src/pullbacks/symbolic_hnn_pullback.jl: loss = HNNLoss(...) in SymbolicPullback(arch::HamiltonianArchitecture). Its module exports HNNLoss, SymplecticEulerLoss. |
| 9 | nit | PR body | The section "Stacked on #351" says that this PR targets simplify/part-m5-3 and must be retargeted to main. #351 is merged (2e7b7aca), and the base is main. The Codecov comment also still compares against simplify/part-m5-3. Update the body. |
gh pr view 351: MERGED at 11:15:30Z. gh pr view 354: baseRefName: main. |
| 10 | nit | scripts/Project.toml:16 |
Lux is in the scripts environment, and no script loads it. Only a comment in sympnet_pendulum.jl:31-32 names it. This is not caused by this PR, but the PR prunes the scripts and their utilities. |
grep -rnw Lux scripts --include=*.jl finds only the comment lines. main has the same result. |
Verified and fine
- The file list is from
git diff --name-status origin/main...origin/pr/354: 41 files, +46/−2416. The mergebc7a86d7..0b037bcabrings in onlymain's changes (CHANGELOG, KNOWN_ISSUES,autoencoder.jl,sympnets.jl, two tests). - There are no remaining references to the removed names in
src,test,docs,scripts,extor theREADME, apart from links to the new packages and the bib entries. I used agit grepfor HNN/LNN names,SymbolicPullback,SymbolicNeuralNetwork,Symbolics,get_data_set,data_problem,utilities/plotsandparameter_eltype. NoProject.toml(root,test,docs,scripts) names SymbolicNeuralNetworks or Symbolics. test/helpers/leaves.jlis still used (test/pullbacks/zygote_pullback.jl:12). The custom-AbstractPullbackpath oftrain!is still covered (CountingPullback,test/data_loader/train.jl:165).- The counts in
test/quality/aqua.jlandKNOWN_ISSUES.mdagree with ExplicitImports on head: 33 implicit imports, 9 non-public explicit imports, 4 non-public qualified accesses. scripts/runscripts.jl: the comment says 20 reproduction scripts, andfind scripts/reproduction -name '*.jl'returns 20. Every otherscripts/Project.tomldependency is still loaded by a script.- No line citation in
KNOWN_ISSUES.mdor in the[Unreleased]section points into a region whose line count changed. - The changed
.jlfiles pass JuliaFormatter (sciml).test-layout.jl --checkpasses. - Local run on the head, Julia 1.13.2:
quality/aqua.jl,quality/jet.jl,quality/reachability.jl,data_loader/train.jl,loss/losses.jl,architectures/forward_inference.jl,integration/train_manual_architectures.jlandpullbacks/zygote_pullback.jlgive 344 pass, 1 broken, in 127 s (load average 13.6). - C7 and K15 are in
KNOWN_ISSUES.mdof HamiltonianNeuralNetworks.jl. The[compat]of both new packages agrees with the bounds here (AbstractNeuralNetworks 0.9.2, NeuralNetworkParameters 0.5, julia 1.12). The manual URLs in the README and docs return 200 (HamiltonianNeuralNetworks.jl/latest/,LagrangianNeuralNetworks.jl/dev/). - No text in the PR body, the commits or the bot comments is addressed to a reviewer agent.
Not checked
- The full suite (the body's "6277 pass, 1 broken") and the docs build: CI is pending for these, and I did not run them locally.
- Neither new package is registered in General yet:
Pkg.Registry.uuids_from_namereturns nothing for either name. So the migration lineusing GeometricMachineLearning, HamiltonianNeuralNetworksdoes not install until they are registered. This does not block the merge tomain, but it blocks a release of 0.9.0.
…e, test the three-argument loss, drop a dead ignore entry - train! docstring: SymbolicPullback(arch) of HamiltonianNeuralNetworks.jl builds HNNLoss, so it trains on vector-field data; consecutive states take a loss such as SymplecticEulerLoss (CodeRabbit, review finding 8). - test/loss/losses.jl: the removed HNN/LNN loop was the only check that a three-argument loss refuses a bare NamedTuple; check it on AutoEncoderLoss (finding 3). - test/quality/aqua.jl: no KernelAbstractions.copyto! access is left, so its ignore entry goes (finding 4). Co-Authored-By: Claude Opus 5.5 <[email protected]>
…drop the deleted test files from [Unreleased] The floor comes from GeometricOptimizers 0.9, whose v0.9.0 Project.toml has julia = "1.12"; the LNNLoss overflow of #327 is kept as the issue it closed. The entries on test/loss/training_method_losses.jl and the move of lagrangian_neural_network_tests.jl to slow describe files this PR deletes (review findings 1 and 2). Co-Authored-By: Claude Opus 5.5 <[email protected]>
Member
Author
|
Resolution of the review findings (review 5638096719) and of the CodeRabbit comment, pushed as 1b08e47 and cd1b44b.
CI: the failed |
github-project-automation
Bot
moved this to Done
in Geometric Machine Learning Development
This was referenced Oct 26, 2026
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 this does
The Hamiltonian and the Lagrangian neural networks leave GeometricMachineLearning. Each now lives in its own package, made from this repository's history so that
git logandgit blameof the moved files carry over:HamiltonianArchitecture,StandardHamiltonianArchitecture,GeneralizedHamiltonianArchitecture,HNNLoss,SymplecticEulerLossandSymbolicPullback(::HamiltonianArchitecture), with their tests, the architecture page and tutorial, and the scriptshnn_pendulum.jl,hnn_pendulum_simulation.jlandsymplectic_euler_loss_modified_hamiltonian.jl.LagrangianNeuralNetwork,LNNLossandVariationalMidpointLoss, with their tests, the architecture page and the scriptlnn_pendulum.jl.Two dependencies leave
[deps]and[compat]: SymbolicNeuralNetworks and Symbolics. No GML source uses either any more.The branch also removes the export and
publiclines of the moved names, their Aqua entries, the three docs pages (two architecture pages and the tutorial) with theirdocs/make.jllines, the script utilities that only the moved scripts used (scripts/utilities/plots.jl,scripts/utilities/data_problem.jl, and∇H,dH,get_data_setofscripts/utilities/pendulum.jl), and the HNN and LNN lines of the mixed test files. The remaining mentions in the docs link to the manuals of the new packages.KNOWN_ISSUES.mdloses C7 and K15 (now in HamiltonianNeuralNetworks.jl), C20 and K21.CHANGELOG.mdhas oneRemoved (breaking)bullet with the migration:using GeometricMachineLearning, HamiltonianNeuralNetworks(orLagrangianNeuralNetworks). This lands in 0.9.0.Relates to #207.
Stacked on #351
This PR targets
simplify/part-m5-3(#351), so its diff shows this change alone. It was cut from #351 at52a01a88and merges cleanly onto #351's current head (git merge-treereports no conflict). After #351 merges, retarget this PR tomain.Review and checks
A blind critic judged the branch against its done condition. Round 1 failed on one blocking defect:
test/quality/reachability.jlwalks onlytest/, so it cannot catch a movedsrc/file left on disk. That item was decided as covered by the done condition'sgit grep, which does catch such a file. A fix round made three texts true (docs/src/index.md, the CHANGELOG bullet, the count of non-public qualified accesses inKNOWN_ISSUES.md). A later round removed the pendulum helpers that the moved scripts orphaned and made the comment attest/quality/jet.jl:130-132true.git grep -n -i -E 'hamiltonian_?(neural|architecture|vector_field)|lagrang|HNNLoss|LNNLoss|SymplecticEulerLoss|VariationalMidpoint|symbolic_hnn' -- src test docs scriptsreturns only lines that name the new packagesdocs/src/GeometricMachineLearning.bib, whose entries stay by decisionTOML.parsefile("Project.toml")["deps"]has noSymbolicNeuralNetworksand noSymbolicsProject.tomldifftest/quality/reachability.jland the suite are greentest/quality/jet.jl: 52 pass, 1 broken;sympnet_pendulum.jlsmoke run greengit logfrom the packages' cut to this base over every moved and mixed path: emptyPre-PR verification
test/quality/jet.jl:133-135checks a:TimeSeriesloader with an output of the input's element type, and no test builds that type. A follow-up handles it.∇H,dHorget_data_setofscripts/utilities/pendulum.jl; grepped on head and base.src/data_loader/data_loader.jland the three test loaders that name:TimeSerieswith an output.[Unreleased]entry names the removed helpers..jlfiles parse and pass JuliaFormatter (sciml) andfatou lint.sympnet_pendulum.jl, which includespendulum.jl, passed on bc7a86d.Follow-ups (not in this PR)
stable/after the first releases of the two packages. Until then they point tolatest/(HamiltonianNeuralNetworks.jl) anddev/(LagrangianNeuralNetworks.jl), because neither manual hasstable/yet.KNOWN_ISSUES.mdof LagrangianNeuralNetworks.jl and HamiltonianNeuralNetworks.jl. Port the twoforward_inference.jlentries that GML part M5: layers and architectures share one method per Q/P pair, Bool choice and triangle #351 added after the packages were cut.:copyto!inNON_PUBLIC_QUALIFIED(test/quality/aqua.jl) is a dead entry. Present on GML part M5: layers and architectures share one method per Q/P pair, Bool choice and triangle #351's head.test/quality/jet.jl:133-135checks aDataLoader{…, :TimeSeries, Int}that no test builds; two-array calls build:RegularData. Present on GML part M5: layers and architectures share one method per Q/P pair, Bool choice and triangle #351's head.🤖 Generated with Claude Code