Repository navigation
P33: the CairoMakie extension, Aqua, docs and CHANGELOG of ReducedBasisMethods - #56
Merged
Merged
Conversation
The singular-value or eigenvalue spectrum of a reduced basis, drawn as it is stored: an EVD spectrum from a Gram matrix and a singular-value spectrum differ by a square, and reading one as the other flatters the decay by a factor of two. CairoMakie is a weak dependency and the method lives in the package extension ReducedBasisMethodsCairoMakieExt, so the package loads with no plotting stack at all and spectrumplot raises the usual MethodError until CairoMakie is loaded. Values at or below zero -- an exact zero of a rank-deficient snapshot, or the slightly negative eigenvalue the Gram route returns -- are lifted to eps(T) * maximum(sigma) so a log-scaled axis draws the whole spectrum; every positive value is drawn as it is given.
One test per check of the part's benchmark: - diagnostics.jl: the headless render of a 10-value spectrum to a PNG, with the plotted y equal to sigma exactly in the precision the plot stores, and a rank-deficient spectrum whose exact zeros and slightly negative eigenvalues are drawn at the floor. - integration/load_without_makie.jl: the package loads in a fresh temporary project that holds only it, no Makie UUID reaches Pkg.dependencies(), the extension is not loaded, and the stub raises the usual MethodError. - quality/aqua.jl: Aqua.test_all is wrapped in a testset, so the passes of its checks are reported rather than swallowed, and stale_deps, undefined_exports and the weakdeps deps_compat entry are visible. - quality/docs.jl: the words a generic package must not write, the doctest count, the entry the docs environment no longer resolves, the changelog entry, and the test environment taking its CairoMakie bound from the package alone. - quality/doctests.jl, in the slow group, as ReducedComplexityModeling does.
get_DEIM_interpolation_matrix is a generic function and its integer basis case is exact, so its docstring now carries a jldoctest: a build with zero doctests passes vacuously, and nothing else in the package asserts that the rendered output of a docstring is what Documenter will show. docs/make.jl loads Weave and docs/Project.toml resolves it, although no page uses either. Both go, and the docs build sets doctest = true so the failure is reported by the build rather than by a warning inside it.
The entry names the extension and the function, and corrects the two bullets this branch makes false: the docs environment no longer resolves Weave or Plots, and the Aqua call is wrapped rather than bare. No earlier bullet is touched.
The plot stores Float64, so the spectrum is widened to Float64 and the floor eps(Float64) * maximum(sigma) is taken in that precision: a Float32 spectrum is widened by an exact widening, and max lifts a rank-deficient spectrum's exact zeros and the Gram route's slightly negative eigenvalues onto the floor, which keeps the drawn order as well. A spectrum with no positive value is refused with an ArgumentError, because a log-scaled axis has no scale to draw it on, and the method returns a FigureAxisPlot so a test reads the plotted data.
The check-5 grep of docs/build could not fail: it ran only where a build already existed, so a word that reached the site alone - through sitename, say - was invisible to a fresh clone and to every CI test job, and where a stale build existed the test searched that instead of the tree under test. The build now greps the site it has just written and errors on a match, so the Documentation workflow is red rather than silent, and the test greps the sources together with docs/make.jl, where such a word has to be written. The words and the search live in one file with two callers, as doctestsetup.jl does.
…hangelog get_DEIM_interpolation_matrix refuses a basis whose columns are exact combinations of the ones before them - its residual is zero, which is the ArgumentError _deim_indices throws - so the docstring no longer describes such an input as one it takes, and it no longer speaks of the test. A bare test_all does record its passes, so the wrapper's reason is the clause's own requirement rather than a claim the base contradicted. The load-without-Makie subprocess adds JULIA_LOAD_PATH to the environment it inherits, where the clause asks for that variable alone, instead of replacing the environment with it. The changelog states the decided rule, the return type, the refusal and the Aqua wrapper as they are.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
+ Coverage 91.56% 91.69% +0.12%
==========================================
Files 10 11 +1
Lines 332 337 +5
==========================================
+ Hits 304 309 +5
Misses 28 28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
michakraus
commented
Oct 14, 2026
michakraus
left a comment
michakraus
left a comment
Member
Author
There was a problem hiding this comment.
JuliaRCM/ReducedBasisMethods.jl PR #56 — P33: the CairoMakie extension, Aqua, docs and CHANGELOG
Verdict: request changes (posted as a comment review)
Reviewed: f4366f11b0d1cec0f13bd5793da233f0e31de1d0
CI, run 38606275071, read per job at review time: Julia 1 - windows-latest and Julia min - windows-latest fail in julia-runtest. In both, the testset Load without Makie errors with ArgumentError: Package Pkg not found in current path (finding 1). The run stops there, so Diagnostics never runs on Windows; the core testsets before it pass. Julia 1 ubuntu and macOS, Julia min ubuntu and macOS, pre, nightly, Doctests and Documentation pass. Downgrade was still running.
Review focus: code quality — fragile, verbose, inefficient and overcomplicated code, and where the new code can be simplified.
Findings
| # | severity | file:line | claim | evidence |
|---|---|---|---|---|
| 1 | blocker | test/integration/load_without_makie.jl:46 |
"JULIA_LOAD_PATH" => "@:@stdlib" uses : as the separator. Julia splits JULIA_LOAD_PATH on ; on Windows, so the subprocess gets the one entry @:@stdlib, cannot load Pkg, and the testset errors. A required check is red. |
Jobs 115997872346 (1.13.1) and 115997872470 (1.11.9), same log: ERROR: ArgumentError: Package Pkg not found in current path., then `Load without Makie |
| 2 | bug | test/quality/docs.jl:52-63 |
The changelog testset fails at the next release. The release commit renames ## [Unreleased] … to the version heading, so findfirst returns nothing, @test head !== nothing fails, and last(head) then throws. |
git show 8cf213b -- CHANGELOG.md (Release 0.2.0): -## [Unreleased] — targeting 0.2.0 / +## [0.2.0] — 2026-10-12. Fix: delete the testset. A CHANGELOG entry is reviewed, not tested. |
| 3 | bug | ext/ReducedBasisMethodsCairoMakieExt.jl:25, src/diagnostics.jl:4,30 |
The documented call spectrumplot(b.Λ) draws Λ = σ² under the axis label σᵢ. The docstring itself says that reading one spectrum as the other "flatters the decay by a factor of two"; the label does that. The first line, "The singular-value spectrum σ", contradicts the example two paragraphs later. |
src/algorithms/svd.jl:39: Λ = σ .^ 2; src/algorithms/evd.jl:26: Λ = reverse(F.values) of the Gram matrix. So b.Λ is the squared spectrum on every route. Fix: a ylabel keyword with a neutral default, and a first line that says "a spectrum: singular values, or the Gram eigenvalues b.Λ". |
| 4 | correctness-risk | src/algorithms/deim.jl:66-69 |
The new paragraph says that a column that is "an exact combination of the ones before it is refused". In floating point the residual of such a column is round-off, not zero, and the column is accepted. The paragraph also repeats the _deim_indices docstring (deim.jl:13-15). |
Probe: 200 draws of Ψ = [x y (0.3 .* x .+ 0.7 .* y)] with x, y = randn(Xoshiro(1), 8): 195 of 200 accepted by get_DEIM_interpolation_matrix, 5 refused. Fix: delete lines 66-69 and keep the doctest. |
| 5 | quality | ext/ReducedBasisMethodsCairoMakieExt.jl:6-28 |
This can be much simpler. Makie's non-mutating lines returns a FigureAxisPlot itself. The method then needs no Figure, Axis or Label, and no call to the non-public constructor CairoMakie.Makie.FigureAxisPlot. any(>(0), σ) is false for an empty σ, so the isempty check is not needed. resolution is Makie's deprecated name for size; the sibling GeometricBracketsMakieExt takes size and keeps resolution only as an alias. The nothing sentinel is not needed either. The simpler form is below the table. |
Makie 0.24.16 src/figureplotting.jl:406: figurelike_return(fa::FigureAxis, plot::AbstractPlot) = FigureAxisPlot(fa.figure, fa.axis, plot). src/scenes.jl:264: "The resolution keyword for Scenes and Figures has been deprecated. Use Figure(; size = ...". |
| 6 | quality | test/integration/load_without_makie.jl:13-53 |
This file does more than it needs to. Lines 14-15 assert that two arbitrary names (Plots, LaTeXStrings) are absent. The subprocess activates a temporary environment and Pkg.develops the package, which is a resolve on every run. It then prints four KEY=value lines and parses them back. MAKIE and MAKIEWEAKDEP check the same fact. catch error (line 40) shadows Base.error. Lines 16-19 already pin [deps], [weakdeps] and [extensions]. The claim, "loading the package loads no Makie", needs one subprocess in the active test environment (below the table). |
Local probe in an environment with CairoMakie installed: using ReducedBasisMethods; print(any(k -> occursin("Makie", k.name), keys(Base.loaded_modules)), " ", Base.get_extension(ReducedBasisMethods, :ReducedBasisMethodsCairoMakieExt) === nothing) printed false true. |
| 7 | quality | test/diagnostics.jl:73-159 |
Four testsets assert drawn == max.(Λ, floor) (lines 90, 111, 126, 157). The expected value is the implementation's own expression (ext/…:22), so these tests pass by construction. They differ only in how a random Gram spectrum with a value below the floor is made. One testset with a hand-written spectrum and a hand-written expected vector tests the floor in both precisions (below the table). |
Lines 90, 111, 126, 157 read; ext/ReducedBasisMethodsCairoMakieExt.jl:22 read. |
| 8 | quality | test/diagnostics.jl:8-21, 43-47 |
The withenv("DISPLAY" => nothing) wrappers guard against nothing: no file in Makie or CairoMakie reads DISPLAY. Lines 43 and 44 assert the same equality. Line 47 asserts a property of the test's own input. |
grep -rn DISPLAY ~/.julia/packages/{CairoMakie,Makie}/*/src: no match. Fix: call spectrumplot directly, write save(joinpath(mktempdir(), "s.png"), fap) inline, and delete lines 44 and 47. |
| 9 | quality | docs/grep_physical_words.jl, docs/make.jl:23-29, test/quality/docs.jl:5-8, 20-33 |
This is overbuilt and fragile. A shared file is included by the docs build and by the suite through pkgdir, and the build greps its own output. The site's text comes from the docstrings, docs/src and docs/make.jl, which the suite already greps. So the build grep can fail only when Documenter itself writes one of the words, and its assets hold none. The suite then asserts the source text of docs/make.jl by regex (lines 31-32): a reflow or a renamed variable breaks that with no change in behaviour. The word plasma matches the authors' affiliation, so a docs page that lists the authors fails the build. ext/ is package source and is not grepped. |
grep -rliE "vlasov|plasma|particle" ~/.julia/packages/Documenter/*/assets: no match. AUTHORS.md:8: "Max Planck Institute for Plasma Physics". Fix: delete docs/grep_physical_words.jl and the make.jl block, and keep one testset (below the table). |
| 10 | quality | docs/make.jl:20, test/quality/docs.jl:43, CHANGELOG.md:66 |
doctest = true is Documenter's default. The line does nothing, the test pins it by string match, and the CHANGELOG announces it as a change. Documenter.yml also says that the doctests run in the Doctests job and not in the docs build. |
Documenter 1.19 src/documents.jl:425: doctest::Union{Bool, Symbol} = true. Fix: remove all three. |
| 11 | quality | test/quality/aqua.jl:5-13, CHANGELOG.md:62-64 |
The wrapper puts a second "Aqua" testset inside @safetestset "Aqua" (runtests.jl:6), which already gives Aqua.test_all a testset. The vacuity check reads the non-public Test.get_test_counts. No other package here does either. The CHANGELOG paragraph describes as new the per-check results that the @safetestset already gave. |
Base.ispublic(Test, :get_test_counts) is false on 1.13.1. grep -rn get_test_counts Packages/*/test: no match. Fix: Aqua.test_all(ReducedBasisMethods) alone, as on main, and drop the paragraph. |
| 12 | quality | test/quality/docs.jl:65-71 |
The testset checks what test/Project.toml contains. That is a rule about the repository layout, not package behaviour. |
Read. Fix: delete it. |
| 13 | quality | src/diagnostics.jl:1-34, CHANGELOG.md:44-60 |
Both are verbose. The docstring is 34 lines for a single plot call. It does not document its keywords (the signature says kwargs...), and it says save "writes it to a PNG", but the extension of the path sets the format. The CHANGELOG entry repeats the docstring nearly word for word over 17 lines. |
Read. Fix: a docstring with the signature and its keywords and three sentences (what is drawn, the floor, what throws), and a CHANGELOG entry of three lines. |
| 14 | nit | test/quality/doctests.jl:5-6, 13 |
The comment says "matching the CI Doctests job", but manual = false differs from the job's doctest(m), and from the sibling file, which calls doctest(GeometricBrackets). using Test is not used. |
.github/workflows/CI.yml, Doctests step: doctest(m). GeometricBrackets/test/quality/doctests.jl read. |
| 15 | nit | test/diagnostics.jl:35,74, test/quality/docs.jl:21, docs/grep_physical_words.jl:2-3, CHANGELOG.md:62 |
Comments cite "check 2", "check 3", "check 4", "check 5" and "T7.15, section 3.6 of the plan". The plan is not published, so a reader of the repository cannot follow these references. | Read. |
| 16 | nit | Project.toml:15 |
The extension uses only Makie API (Figure, Axis, Label, lines!) but loads only with CairoMakie, so a GLMakie user must also load CairoMakie. GeometricBrackets follows the same convention, so this is a note, not a request. |
ext/ReducedBasisMethodsCairoMakieExt.jl read. |
Simpler forms
Finding 5: the whole extension method.
function ReducedBasisMethods.spectrumplot(σ::AbstractVector{<:Real};
title = "", ylabel = "", size = (620, 380))
all(isfinite, σ) || throw(ArgumentError("the spectrum holds a NaN or an Inf"))
any(>(0), σ) || throw(ArgumentError("the spectrum holds no positive value"))
y = max.(Float64.(σ), eps(Float64) * Float64(maximum(σ)))
return lines(y; figure = (; size),
axis = (; title, xlabel = "i", ylabel, yscale = log10))
endFinding 6: the subprocess, with no Pkg, no load path and no protocol.
code = """
using ReducedBasisMethods
print(any(k -> occursin("Makie", k.name), keys(Base.loaded_modules)), " ",
Base.get_extension(ReducedBasisMethods, :ReducedBasisMethodsCairoMakieExt) === nothing)
"""
@test readchomp(`$(Base.julia_cmd()) --startup-file=no --project=$(Base.active_project()) -e $code`) ==
"false true"Finding 7: one testset, with the expected values written out.
@testset "values at or below the floor are drawn at the floor ($T)" for T in (Float64, Float32)
σ = T[1, 1e-3, 1e-20, 0, -1e-18]
@test plotted_y(spectrumplot(σ)) == [1, Float64(T(1e-3)), eps(), eps(), eps()]
endFinding 9: the whole word check, over ext/ as well, and without plasma, which names the authors' institute.
@testset "no physical word in the package" begin
root = pkgdir(ReducedBasisMethods)
for dir in ("src", "ext", "docs/src"), (path, _, files) in walkdir(joinpath(root, dir)),
file in files
@test !occursin(r"vlasov|particle"i, read(joinpath(path, file), String))
end
endVerified and fine
- Formatting: JuliaFormatter with the repository's
scimlconfig, over the 12 changed.jlfiles: all formatted. test-layout.jl --check: no violations.- The package loads with no Makie module, in a local environment that has
CairoMakieinstalled (finding 6, probe outputfalse true). - The doctest output
get_DEIM_interpolation_matrix([1 1; 2 -1]) == [0.0 1.0; 1.0 0.0]reproduces. - Documenter's bundled assets hold none of the three words.
closingIssuesReferencesis empty. Nothing in the PR text is addressed to a reviewer or an agent.
Not checked
- The full suite and the docs build, run locally. The PR's pass counts (3097) were not measured again.
- Whether
fap.plot.positions[]and its element type stay stable across Makie 0.24 patch releases. - The result of
Downgrade, which was still running. - The rendered output of the simpler forms above: the local CairoMakie precompile did not finish during the review. They are written from the Makie source cited in finding 5 and were not run.
…he docs checks Resolves the findings of the review of PR #56: - load_without_makie.jl joins JULIA_LOAD_PATH with the platform separator, so the Windows subprocess finds Pkg, and runs its checks as @test inside the subprocess instead of printing KEY=value lines and parsing them back. - spectrumplot is a single lines() call that returns the FigureAxisPlot. It no longer labels the y axis σᵢ, because b.Λ holds squared singular values. The docstring documents the keywords and is a third of its length. - The four floor testsets that compared against the implementation's own max.(Λ, floor) are one testset with hand-written expected values. The DISPLAY wrappers, which had no effect, are gone. - The changelog test searches the whole file, so it survives a release. The physical-word grep covers ext/ too, and the shared grep file, the grep of the built site and the regex test of docs/make.jl are gone. doctest = true, Documenter's default, is dropped. The layout testset repeated test-layout.jl's compat rule and is gone. - aqua.jl is the bare Aqua.test_all again, without the non-public Test.get_test_counts. - The DEIM docstring loses a paragraph that called an exactly dependent column refused: 195 of 200 such bases are accepted in floating point. - The CHANGELOG entry states the branch as it is. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Resolve the conflicts with the 0.2.1 release and get_DEIM_indices (#57): the spectrumplot entry moves to a new Unreleased section, and the DEIM doctest follows the renamed reference. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Member
Author
|
Resolution of the review findings (review), all in commit 2ca1428.
Verification on Julia 1.13.1, macOS: the six affected test files ( While this round ran, |
michakraus
commented
Oct 14, 2026
michakraus
left a comment
michakraus
left a comment
Member
Author
There was a problem hiding this comment.
JuliaRCM/ReducedBasisMethods.jl PR #56 — P33: the CairoMakie extension, Aqua, docs and CHANGELOG (second round)
Verdict: request changes (posted as a comment review)
Reviewed: f19fe3fd72cc781fe0f2ef9394f5d34fa116f43a
CI: green on the reviewed head, read per job in run 38612335137 (headSha f19fe3f). The seven required checks pass: Julia min and Julia 1 on ubuntu, macOS and windows, and Doctests - ubuntu-latest. Each test job ends in ReducedBasisMethods tests passed; Load without Makie | 4 4 and Diagnostics | 21 21 on both Windows jobs, which the first round saw fail. The advisory pre, nightly and Downgrade pass too. Doctests reports Doctests: ReducedBasisMethods | 1 1. Documentation and both codecov checks pass.
Review focus: code quality. The review looks for fragile, verbose, inefficient and overcomplicated code, and for places where the new code can be much simpler.
Findings
Findings 1–4 are the points the first round left open (first-round findings 6, 5, 3 and 16). The user has decided them, and each row gives the evidence.
| # | severity | file:line | claim | evidence |
|---|---|---|---|---|
| 1 | quality | test/integration/load_without_makie.jl:9-26 |
(First-round 6.) A simpler check proves the claim. The resolution says that only a fresh project "shows no Makie package is installed". The package's own [deps] cannot hold Makie: using ReducedBasisMethods would load it, and Aqua stale_deps fails on a dependency that is listed but not loaded. A hard dependency further down would also load Makie. A subprocess in the active test environment sees both cases through Base.loaded_modules. The fresh project adds one case only: a third-party dependency that lists Makie in its [deps] and never loads it. For that case, every run pays a full Pkg.develop resolve and needs a registry. The Downgrade job also resolves the newest versions there, not the floors it tests. The simpler file needs no Pkg, no load-path separator and no TOML block. The get_extension(...) === nothing line goes as well: it passes for any extension name, so it is vacuous after a rename (finding 4). |
Cold runs, PR file against the simpler file below, same environment, two runs each: Load without Makie | 4 4 5.0s and 4.9s, against | 1 1 1.4s and 1.4s. The simple check can fail: with using ReducedBasisMethods, CairoMakie, any(m -> occursin("Makie", m.name), keys(Base.loaded_modules)) and get_extension(...) === nothing print true false. CI times of this testset: 8.7 s to 10.7 s on the matrix jobs, 20.4 s on Downgrade. Machine load during the runs: about 5. |
| 2 | quality | ext/ReducedBasisMethodsCairoMakieExt.jl:7,15, src/diagnostics.jl:2, CHANGELOG.md:21 |
(First-round 5.) Rename the keyword resolution to size. This is Makie's current name, and the sibling GeometricBracketsMakieExt uses it. The method then passes figure = (; size). |
Makie 0.24.16 src/scenes.jl:264: "The resolution keyword for Scenes and Figures has been deprecated. Use Figure(; size = ...". The form below was probed: size = (300, 200) gives Base.size(fap.figure.scene) == (300, 200). |
| 3 | quality | ext/ReducedBasisMethodsCairoMakieExt.jl:7,16, src/diagnostics.jl:2-6 |
(First-round 3.) Add a ylabel keyword with the neutral default "", and document it. Then the caller can say whether σ holds singular values or Gram eigenvalues, without reaching into fap.axis. |
Probe of the form below: ylabel = "λᵢ" gives fap.axis.ylabel[] == "λᵢ", and the default gives "". |
| 4 | quality | Project.toml:14-21, ext/ReducedBasisMethodsCairoMakieExt.jl:1-3 |
(First-round 16.) Make Makie the trigger: [weakdeps] Makie, [extensions] ReducedBasisMethodsMakieExt = "Makie", [compat] Makie = "0.24", and using Makie in the extension. The extension calls only lines. save comes from FileIO and needs a backend where it is called, which the test and the user load. This review found no technical reason against it. The one reason against is consistency: GeometricBrackets triggers on CairoMakie. Follow-on edits: CairoMakie becomes a test-only dependency and needs its own bound in test/Project.toml (CairoMakie = "0.15"), because the layout rule gives the package's bound only to a shared dependency. The extension's name changes in load_without_makie.jl:11,21, in CHANGELOG.md:20-31, and in the docstring's "load CairoMakie" (src/diagnostics.jl:12). |
Probe: a copy of the head with these edits, in an environment that has only CairoMakie, Aqua, Test and the package as direct dependencies. Output: ReducedBasisMethods → ReducedBasisMethodsMakieExt precompiles, ext loaded: true, png: true from save, Aqua | 11 11. parentmodule(Makie.save) is FileIO. Every installed CairoMakie 0.15 pins Makie = "=0.24.x", and 0.15.0 pins =0.24.0, so "0.24" has the same floor as today's "0.15". GeometricBrackets/Project.toml: GeometricBracketsMakieExt = "CairoMakie". docs/Project.toml holds no Makie package, so the docs need none. |
| 5 | quality | test/quality/docs.jl:23-38 |
Three of the four testsets test what files contain, not what the package does. That is the class of first-round finding 12, whose testset was deleted. (a) Lines 23-26 check docs/Project.toml for Weave and Plots. (b) Lines 28-32 check that the source text holds a jldoctest fence. The Doctests job already reports the count (Doctests: ReducedBasisMethods | 1 1), and removing the doctest is a reviewed diff. (c) Lines 34-38 are first-round finding 2, "fixed" so that the test cannot fail: after the release, both words stay in the 0.2.2 section for good. Delete the three testsets. using TOML and files_matching then go too, and the file is the word check below. plasma also leaves the regex: it matches the authors' institute (AUTHORS.md:8,10,12), so a docs page that lists the authors fails. |
File read. CI log of the Doctests job. The simple form below was run: | 16 16. |
| 6 | quality | test/diagnostics.jl:3, 38, 40, 42-46 |
Lines 42-46 build a ReducedBasis from a zero snapshot set to get an all-zero Λ. spectrumplot sees only the vector, so the case repeats spectrumplot(zeros(3)) on line 39. Line 45, @test all(iszero, Λ), asserts the test's own input. That is the same kind of line as the one first-round finding 8 removed. Line 38 (-Inf) repeats line 37, and line 40 (Float32[0, 0]) repeats line 39: the checks are all(isfinite, σ) and any(>(0), σ), which do not depend on the sign of an Inf or on the element type. Delete lines 38, 40 and 42-46, and the ReducedComplexityModeling import on line 3. |
ext/ReducedBasisMethodsCairoMakieExt.jl:8-9 read. |
| 7 | quality | test/quality/doctests.jl:4-10 |
manual = false makes two of the setup lines dead. @eval Main import exists for the manual pages' @meta blocks (the sibling GeometricBrackets/test/quality/doctests.jl says so). setdocmeta! repeats julia> using ReducedBasisMethods in the one doctest (deim.jl:71). The comment "Documenter evaluates page metadata in Main" then describes nothing that this call does. Use either the three-line form below, or the sibling's doctest(ReducedBasisMethods) with its setup kept. |
Run through @safetestset, as runtests.jl does: the file without line 7 gives Doctests | 1 1. The three-line form gives Doctests | 1 1. |
| 8 | quality | CHANGELOG.md:20-35 |
The entry is still 16 lines (first-round 13). It repeats the docstring: the floor sentence and the list of refusals. The second paragraph (the first jldoctest, Weave and Plots leave the docs environment) is maintenance filed under ### New Features. The file has a ### Changed heading for it (CHANGELOG.md:165), or the paragraph can go. Findings 2-4 change the entry anyway (resolution, the trigger, the extension's name). |
Read against src/diagnostics.jl:8-10. |
| 9 | quality | PR body | The body describes the first-round head, not this one. Examples: "Aqua inside a named testset", "julia --project=docs docs/make.jl exits 0 with doctest = true", "the build greps its own site", "returns CairoMakie.Makie.FigureAxisPlot(fig, ax, plt)", JULIA_LOAD_PATH="@:@stdlib" with Pkg.dependencies(), and "3097 passes". Commit 2ca1428 removed or replaced all of these. The body also cites an unpublished plan ("§5V.P33", "T7.12", "T7.15", "advisor"). That is the issue of first-round finding 15, which is now fixed in the code but not in the body. The body is the merge record, so rewrite it for the head. |
gh pr view 56 --json body read, against the diff at f19fe3f. |
| 10 | nit | test/diagnostics.jl:6-8, 14-15 |
The docstring "The plotted values of a FigureAxisPlot, and their element type." attaches to plotted_y only. plotted_eltype is used once, on line 15: inline it. |
Read. |
Simpler forms
Findings 2 and 3, the extension method (probed as a free function, with the output given in rows 2 and 3):
function ReducedBasisMethods.spectrumplot(σ::AbstractVector{<:Real};
title = "", ylabel = "", size = (620, 380))
all(isfinite, σ) || throw(ArgumentError("the spectrum holds a NaN or an Inf"))
any(>(0), σ) || throw(ArgumentError("the spectrum holds no positive value"))
# A log-scaled axis cannot draw the exact zeros of a rank-deficient snapshot set or the
# round-off negatives of a Gram spectrum, so every value below the floor is lifted onto it.
y = max.(Float64.(σ), eps(Float64) * Float64(maximum(σ)))
return lines(y; figure = (; size),
axis = (; title, titlealign = :left, xlabel = "i", ylabel, yscale = log10))
endFinding 1, the whole of test/integration/load_without_makie.jl (run: | 1 1 1.4s). The extension's name is then pinned by behaviour in test/diagnostics.jl: @test Base.get_extension(ReducedBasisMethods, :ReducedBasisMethodsMakieExt) isa Module.
using ReducedBasisMethods
using Test
@testset "Load without Makie" begin
code = """
using ReducedBasisMethods, Test
@test !any(m -> occursin("Makie", m.name), keys(Base.loaded_modules))
@test_throws MethodError spectrumplot([1.0, 0.5])
"""
@test success(`$(Base.julia_cmd()) --startup-file=no --project=$(Base.active_project()) -e $code`)
endFinding 5, the whole of test/quality/docs.jl (run: | 16 16). Its @safetestset label in runtests.jl then drops "and changelog".
using ReducedBasisMethods
using Test
@testset "no physical word in the code or the documentation" begin
root = pkgdir(ReducedBasisMethods)
files = [joinpath(path, file) for dir in ("src", "ext", "docs/src")
for (path, _, names) in walkdir(joinpath(root, dir)) for file in names]
for file in [files; joinpath(root, "docs", "make.jl")]
@test !occursin(r"vlasov|particle"i, read(file, String))
end
endFinding 7, the whole of test/quality/doctests.jl (run: Doctests | 1 1):
using ReducedBasisMethods
using Documenter: doctest
doctest(ReducedBasisMethods; manual = false)Verified and fine
The claimed fixes of the resolution comment, checked against f19fe3f:
- 1:
load_without_makie.jl:24joins the load path withSys.iswindows() ? ';' : ':', and both Windows jobs pass the testset. Finding 1 makes the line unnecessary. - 3: the axis carries no
σᵢlabel, andsrc/diagnostics.jl:4-6says that the values are drawn as given. - 4: the
deim.jlparagraph is gone, and only the doctest is added (deim.jl:70-77). - 5: the method is one
linescall. NoFigure,Axis,Label,isemptycheck orFigureAxisPlotconstructor is left. - 7: one testset with hand-written expected values (
diagnostics.jl:28-32). - 8: no
DISPLAYwrapper is left, and no duplicate equality. One input precondition is left (finding 6). - 9:
docs/grep_physical_words.jland the build grep are gone, and the word check coversext/. - 10:
doctest = trueis gone fromdocs/make.jl. - 11:
test/quality/aqua.jlis not in the diff, so it is the same as onmain. - 12: the
test/Project.tomltestset is gone. - 13: the docstring is 19 lines and documents both keywords.
- 14: the comment no longer names the CI job, and
using Testis gone. - 15:
grep -rnE "check [0-9]|T7\.|the plan|P33"matches none of the changed files. The hits are in test files that this PR does not touch.
Other checks:
- Locally, on Julia 1.13.1 with CairoMakie 0.15.16 and Makie 0.24.16:
test/diagnostics.jlgives21 21, andtest/quality/docs.jlgives8 8. - JuliaFormatter with the repository's
scimlconfig, over the 10 changed.jlfiles: all formatted. closingIssuesReferencesis empty. No commit, comment or review thread holds text addressed to a reviewer or an agent. The PR body's "advisor decisions" are design notes, not instructions.- Required checks: branch protection and the ruleset on
main(ruleset 24118221) name the same seven contexts, with strict status checks.
Not checked
- The full suite, run locally. CI ran it on the head, and the resolution comment's counts (115 of 115, 431 of 431) were not measured again.
- How
Downgraderesolves aMakie = "0.24"floor underforcedeps(finding 4). It was not run. fap.plot.positions[]inplotted_y, and the typeMakie.FigureAxisPlotnamed in the docstring and the test, are not public Makie API (Base.ispublic(Makie, :FigureAxisPlot) == false). The first round accepted this. This review did not check how stable they are across Makie 0.24 patch releases.
Second review round of PR 56. The extension becomes ReducedBasisMethodsMakieExt with Makie 0.24 as the weak dependency; CairoMakie is test-only. spectrumplot takes ylabel and size (Makie's name) in place of resolution. The load test runs one subprocess in the active test environment, with no Pkg resolve. docs.jl keeps only the word check, without plasma. doctests.jl drops the setup that manual = false makes dead. diagnostics.jl drops repeated refusals and the zero-snapshot input check, and pins the extension name. The CHANGELOG entry is shortened. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Member
Author
|
Resolution of the second-round review (review). Every finding is fixed: findings 1-8 and 10 in commit 73ff301, finding 9 in the PR body.
Verification on Julia 1.13.1, macOS: |
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
1 participant
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.
A
Makieextension withspectrumplot, a doctest, and test and docs clean-upWhat changes
spectrumplot(σ; title = "", ylabel = "", size = (620, 380))is exported. It draws aspectrum against its index on a log10 axis, in
Float64, and returns aMakie.FigureAxisPlot.The values are drawn as given, so
spectrumplot(b.Λ)draws the Gram eigenvalues andylabelnames which spectrum it is. Every value below
eps(Float64) * maximum(σ)is drawn at that floor,so the exact zeros and the round-off negatives of a Gram spectrum stay on the axis. A
NaN, anInf, or no positive value throws anArgumentError.ReducedBasisMethodsMakieExt, which loads withMakie(
[weakdeps],[compat] Makie = "0.24"). The extension calls onlylines, so any Makie backendworks;
saveneeds one, such asCairoMakie.using ReducedBasisMethodsloads no Makie module,and
spectrumplothas no method untilMakieis loaded.get_DEIM_interpolation_matrixcarries the package's firstjldoctest.WeaveandPlots, which no page used.CHANGELOG.mdhas the entry under[Unreleased].Tests
test/diagnostics.jl(slow): the extension loads under its name; aFloat64and aFloat32spectrum are drawn exactly and saved as a PNG through
CairoMakie;ylabelandsizereach theaxis and the figure; the floor is checked against hand-written values in both precisions; the
refusals throw.
test/integration/load_without_makie.jl(slow): a subprocess in the test environment, which hasCairoMakieinstalled, loads the package and finds no Makie module and nospectrumplotmethod.test/quality/docs.jl(core): no file insrc,ext,docs/srcordocs/make.jlnames aphysical model.
test/quality/doctests.jl(doctests): the docstring doctests.CairoMakieis a test-only dependency, with its own bound intest/Project.toml.Known limitations, not addressed here
fap.plot.positions[], which the tests read, andMakie.FigureAxisPlot, which the docstringnames, are not public Makie API.
scripts/andnotebooks/still usePlotsand name a physical model. This change does nottouch them.
🤖 Generated with Claude Code