Repository navigation
Make the four names HamiltonianNeuralNetworks reaches public and documented - #374
Conversation
…documented _flatten_system_parameters, concatenate_array_with_parameters, assign_q_and_p and _check_forcing_type are declared public, not exported. Their explanatory comments become docstrings that name each method, the return value and the errors, and each docstring is listed in a @docs block. A testset in test/quality/exports.jl asserts all four are public, not exported and carry a non-empty docstring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ames The docstring of assign_q_and_p says that the halves are what indexing returns (ranges for a range) and that N <= 0 gives empty halves. The CHANGELOG entry of apply_parametric no longer calls concatenate_array_with_parameters internal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Findings
 |
| Layer / File(s) | Summary |
|---|---|
Document and register helper APIs src/kernels/assign_q_and_p.jl, src/layers/forcing_layer.jl, src/parametric.jl, src/GeometricMachineLearning.jl, docs/src/architectures/neural_network_integrators.md, docs/src/architectures/sympnet.md, docs/src/arrays/tensors.md, test/quality/exports.jl, CHANGELOG.md |
Docstrings describe the four helper functions. The module declares them public without exporting them. Documentation lists and tests cover their public status and documentation. The changelog records these APIs and describes parametric functionality. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Feature
Merge Risk: 
fa4cb
-
The helper can produce incorrect state partitions for offset-indexed arrays; clarify or fix the indexing contract before merging.
Pre-merge checks: 
-
Check name Status Explanation Title check The title states the main change: four named functions become public and documented so HamiltonianNeuralNetworks.jl can reach them. It matches the changeset (public declarations, docstrings, and @docs…Description check The description says HamiltonianNeuralNetworks.jl uses four qualified names and that the PR declares them public, turns comments into docstrings, adds @docslistings on the three pages, and adds a tes…Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… 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: Title check
-
Explanation
The title states the main change: four named functions become public and documented so HamiltonianNeuralNetworks.jl can reach them. It matches the changeset (public declarations, docstrings, and
@docslistings for those four names), and it is specific enough for a teammate scanning history. It is slightly long at 75 characters.
Full details: Description check
-
Explanation
The description says HamiltonianNeuralNetworks.jl uses four qualified names and that the PR declares them public, turns comments into docstrings, adds
@docslistings on the three pages, and adds a testset in test/quality/exports.jl. This matches the changed files in the raw summary. It is detailed and on-topic.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (9 skipped: 9 unsupported.)
-
- 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.
![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:
c230a919-69e6-466f-b832-79e55c08d280
-
CHANGELOG.mddocs/src/architectures/neural_network_integrators.mddocs/src/architectures/sympnet.mddocs/src/arrays/tensors.mdsrc/GeometricMachineLearning.jlsrc/kernels/assign_q_and_p.jlsrc/layers/forcing_layer.jlsrc/parametric.jltest/quality/exports.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
left a comment
michakraus
left a comment
There was a problem hiding this comment.
JuliaGNI/GeometricMachineLearning.jl PR #374 — Make the four names HamiltonianNeuralNetworks reaches public and documented
Verdict: request changes (posted as a comment review)
Reviewed: fa4cb0af583eaa2705b65d1da295d32712dadaf4
CI: in progress when read. Downgrade (ubuntu) is red at the julia-downgrade-compat step: a resolver check, not a test. The forcedeps lower-bound check fails for NeuralNetworkParameters, Zygote, ChainRulesCore, NNlib and KernelAbstractions. The same job fails at the same step on main at 848d7f2 (run 41225938159), and this PR changes no Project.toml, so the failure does not come from this PR. All other jobs were pending. CodeRabbit passed.
Findings
| # | severity | file:line | claim | evidence |
|---|---|---|---|---|
| 1 | quality | src/GeometricMachineLearning.jl:241-242, src/parametric.jl:26-29, src/layers/forcing_layer.jl:16 |
Two of the four names become public with a leading underscore. In Julia, and in this module, a leading underscore marks an internal name: src/GeometricMachineLearning.jl:141-143 calls _diff, _norm and _vcat "helpers ..., not surface". The PR keeps the underscores because "a rename breaks the qualified access of HamiltonianNeuralNetworks.jl". That does not hold now. After the names are public, a rename is a breaking change. |
HamiltonianNeuralNetworks main pins this package with [sources] GeometricMachineLearning = {url = ..., rev = "2604f115..."} and [compat] GeometricMachineLearning = "0.8". [Unreleased] here already holds breaking changes for 0.9.0. A rename reaches HNN only when it moves its pin, and then it changes 5 call sites: GeometricMachineLearning._flatten_system_parameters in generalized_hamiltonian_neural_network.jl (2), layerwise_seam.jl (1) and symbolic_hnn_pullback.jl (1), and GeometricMachineLearning._check_forcing_type in forced_generalized_hamiltonian_neural_network.jl (1). This package has 5 + 2 call sites. Suggest flatten_system_parameters and check_forcing_type. |
| 2 | nit | src/kernels/assign_q_and_p.jl:5-11 |
The new docstring says "its first N rows" and "a BoundsError when x has fewer than 2N rows". Both are false for an array with an offset first axis, because the body indexes 1:N and (N + 1):(2N). This agrees with the CodeRabbit thread on line 5. The body is unchanged, so the docstring alone needs a fix: name the indices 1:N and N + 1:2N, or say "one-based first axis". |
assign_q_and_p(Base.IdentityUnitRange(0:3), 1) returns (q = 1:1, p = 2:2), and the first row is index 0. assign_q_and_p(Base.IdentityUnitRange(0:3), 2) gives a BoundsError for an input that has 2N = 4 rows. |
| 3 | nit | src/parametric.jl:51-52 |
The docstring lists the inputs with no method, and the list is incomplete. A 3-tensor (or a larger array) with a vector of μ has no method either. |
concatenate_array_with_parameters(rand(2, 3, 4), [(a = 1.0,) for _ in 1:3]) gives a MethodError. |
| 4 | nit | src/kernels/assign_q_and_p.jl:13-14, src/layers/forcing_layer.jl:14, src/parametric.jl:24, src/parametric.jl:54 |
Each of the four docstrings ends with "HamiltonianNeuralNetworks calls this function by its qualified name, so it is public." That sentence gives a maintainer's reason for the visibility. It does not describe the function, and it goes stale when HNN changes. The same fact is also in the comment of the public statement (src/GeometricMachineLearning.jl:239-240), the test comment (test/quality/exports.jl:100) and the CHANGELOG. Keep it in the module comment and the CHANGELOG, and remove it from the docstrings. |
Read at the lines given. |
Verified and fine
- HamiltonianNeuralNetworks
mainreaches exactly these four names by qualified access insrc/, fromgh api .../contentsof its five source files. The other names it takes throughusing GeometricMachineLearning: ForcingLayer, ParametricLoss, apply_parametricare public already. - The diff changes no function body: only docstrings, comments, one
publicstatement, three@docsentries, a CHANGELOG bullet and the testset. test/quality/exports.jlpasses 56/56 on Julia 1.13.3 against the worktree at this SHA. The count of 12 failures onmainmatches 4 names × 3 assertions that need the change (ispublic,hasdoc, text). I did not run it onmain.- Every claim of the four docstrings except the ones in findings 2 and 3 holds when called:
_flatten_system_parametersreturns(Vector, NestedLayout). WithoutT, the element type isUnion{}forNullParameters()and(;). WithT, it gives aVector{T}. AStringleaf and two leaf element types (Float64/Float32,Int/Float64) give anArgumentError.0.5withT = Intgives anInexactError. The layoutisa NeuralNetworkParameters.ParameterLayout.concatenate_array_with_parametersreturnsxitself (===) forNullParameters(). It givesFloat64rows below a vector, a matrix and a 3-tensor, and aFloat32result for aFloat32tensor with aFloat64μ. A vector ofμgoes column by column. A length mismatch gives aDimensionMismatch. An emptyμswith zero columns gives anArgumentError.0.5below anIntstate gives anInexactError. A 4-D array with aNamedTuple, and a vector with a vector ofμ, give aMethodError.assign_q_and_pcopies for anArrayand for a view. It returns ranges for a range.N = 0andN = -1give empty halves. It gives aBoundsErrorfor fewer than2Nrows, anArgumentErrorfor a 0-dimensional array, and ignores the rows after2N._check_forcing_typereturns:QP, gives anArgumentErrorfor:X, and aMethodErrorfor aString.
- JuliaFormatter (
.JuliaFormatter.toml, sciml) reports the five changed.jlfiles as formatted. - The PR closes no issue (
closingIssuesReferencesis empty). The PR text gives no instruction to a reviewer or an agent.
Not checked
- The docs build. The Documentation job for this SHA was pending. It passed on
ba75513(workflow_dispatch run 41227795481), andfa4cb0aafter it changes only docstring and CHANGELOG text. - The full suite, Julia 1.12, Aqua and JET. The PR changes no code, only docstrings and visibility.
- assign_q_and_p: name the indices 1:N and N + 1:2N that the body reads, so the text and the BoundsError condition hold for an offset first axis (review finding 2, CodeRabbit's thread). - concatenate_array_with_parameters: list a 3-tensor or a larger array with a vector of parameters among the inputs with no method (finding 3). - Remove "HamiltonianNeuralNetworks calls this function by its qualified name, so it is public." from the four docstrings. The reason for the visibility stays in the comment of the public statement and in the CHANGELOG (finding 4). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Follow-up to the review above. Commit 7052901 resolves each finding as follows:
Checks on 7052901: JuliaFormatter (sciml) reports the three changed files as formatted. |
… underscore The PR makes both names public, and a leading underscore marks an internal name in this module. They become flatten_system_parameters and check_forcing_type, before any release carries the old spelling (review finding 1, decided by the user). The rename covers the definitions, the docstrings, the 5 call sites of flatten_system_parameters and the 2 of check_forcing_type, the public statement and its comment, the @docs entries, the test tuple of test/quality/exports.jl and the [Unreleased] CHANGELOG bullet. HamiltonianNeuralNetworks.jl calls both old names and pins this package by git rev, so it is unaffected until it moves its pin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Follow-up: finding 1 of the review is now fixed in 5e0d8c1, by the maintainer's decision.
Checks on 5e0d8c1: JuliaFormatter (sciml) on the 6 changed |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #374 +/- ##
=======================================
Coverage 94.48% 94.48%
=======================================
Files 63 63
Lines 2738 2738
=======================================
Hits 2587 2587
Misses 151 151 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-project-automation
Bot
moved this to Done
in Geometric Machine Learning Development
HamiltonianNeuralNetworks.jl reaches four functions of this package by qualified access:
_flatten_system_parameters,concatenate_array_with_parameters,assign_q_and_pand_check_forcing_type. This pull request declares thempublic(not exported) in onepublicstatement, turns their explanatory comments into docstrings that name every method, the return value and the errors, and lists each docstring in a@docsblock of the Library Functions section of the neural network integrators, SympNet and tensors pages. No function body changes.A new testset in
test/quality/exports.jlasserts, for each name,Base.ispublic,!Base.isexported,Docs.hasdocand a non-empty docstring text read from the module's docs metadata. It fails onmain(44 pass, 12 fail) and passes on the branch (56/56).Critic's verdict
PASS, judged on 848d7f2...ba75513. The head adds one text-only commit after it (fa4cb0a): the
assign_q_and_pdocstring now says that the halves are what indexing returns (ranges for a range) and thatN ≤ 0gives empty halves, and the CHANGELOG entry ofapply_parametricno longer callsconcatenate_array_with_parametersinternal.maintest/quality/exports.jl: base 44 pass / 12 fail, head 56/56publicBase.ispublic!Base.isexported(4 failures)assign_q_and_pdocstring deletedDocs.hasdocand the text checkArgumentError,InexactError,DimensionMismatch,BoundsError,MethodErrorcases)@docsentry each, at the named placesrc/publicline;FORCING_TYPESkeeps its comment[Unreleased]### AddedKnown issues
The branch adds no
KNOWN_ISSUES.mdentry. The existing K17 (assign_q_and_pdrops the rows from2N + 1on without an error) still holds, and the new docstring states that behaviour.Decisions taken without the user
public, notexport:using GeometricMachineLearningdoes not bring them in.publicstatement for the four, with a comment that says why, beside the existingpublicstatements.FORCING_TYPESkeeps a comment of its own.Every clause is met as written, except 6b, which waits for the checks of this pull request.
Pre-PR verification
Aqua.test_piraciesreports the deliberatetrain!methods atsrc/data_loader/train.jl:34, whichtest/quality/aqua.jldocuments. With the repository'streat_as_own = [Batch]it passes.fatou lintreports nothing. JuliaFormatter (sciml) reports the changed files as formatted.[Unreleased]/Added.[Unreleased]already holds breaking changes for 0.9.0.🤖 Generated with Claude Code