Repository navigation
Build - BUGFIX! - Bump FastInterpolations to v0.4.20 for the Periodic BC fix - #532
Merged
Merged
Conversation
logan-nc
marked this pull request as ready for review
October 23, 2026 05:48
logan-nc
self-requested a review
October 23, 2026 05:48
Restore develop's pins for every other package so the PR moves exactly one dependency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdRsGGg4YRG6E9fgLGbpdF
Reverts 8a4813a: take the full update.jl resolve, not only the FastInterpolations bump. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdRsGGg4YRG6E9fgLGbpdF
logan-nc
reviewed
Oct 23, 2026
logan-nc
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed on behalf of @logan-nc. This is a comment review; the approval stays with a human reviewer.
The numbers look fine. I ran the regression harness on a 2×2 grid: FastInterpolations 0.4.18 or 0.4.20, each with and without #555. The full report is in the PR body.
- The bump moves Solovev energies by about 2e-4 relative and DIII-D-like energies by under 1e-6.
- The Riccati Δ′ diagonal moves by up to 0.15%, SLAYER γ by up to 0.07%, and ODE step counts by under 1%.
- Geometry scalars agree to 1e-7 or better.
- The bump causes the same moves with and without #555, so the two PRs don't interact and can merge in either order.
Manifests. 067f6d20e restores the fully regenerated ci/manifests/ resolves, as CLAUDE.md asks after a [compat] change. Both runtests jobs pass on them.
PR body. I added the ## Release note block with the harness stamp at 067f6d20e, so the "Title and release note" check should now pass once it reruns. I also removed the "This is a draft" line, since #525 is closed.
Nothing blocking from my side.
logan-nc
approved these changes
Oct 24, 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.
Bumps FastInterpolations to v0.4.20, which fixes its periodic boundary condition. Closes #531.
The issue it was waiting on, #525, turned out to be a red herring and is closed, so this PR is no longer blocked.
Release note
Pkg.instantiate()to pick up FastInterpolations v0.4.20. Expect small differences when comparing against outputs from earlier builds.FastInterpolations v0.4.20 fixes a bug in its periodic boundary condition, which GPEC's splines use. Results shift only slightly.
Regression report
Run on alfven, Julia 1.11.7, 32 threads, job 783. Four refs form a 2×2 grid: FastInterpolations 0.4.18 or 0.4.20, each with and without #555 (
singfac_min). Each ref is the named commit plus one commit that pins the rootManifest.tomlto that FastInterpolations version.test/fi20-532(8a39efaf3) has the samesrc/as this PR's head,067f6d20e.fi18-dev→fi20-5329a51446dffi18-555→fi20-532-555Neither PR depends on the other, so they can merge in either order. Geometry scalars (q0, q95, β, volume, the
li) agree to 1e-7 or better. The profile checksums change because they hash bits, not values.ggj_ray_q500ifailed on every ref, because the harness runner in my checkout predates the develop fix that qualifiesInnerLayer.GGJ.q4_surface_benchmark(). It is unrelated to this PR.gal_resistive_pereports N/A for the per-surface PE Δ′ on every ref.Changed rows only (the full report is 87 kB, over GitHub's body limit)
Notes for reviewers
The CI manifests in
ci/manifests/are the fully regenerated resolves (067f6d20e), not a one-package pin.🤖 Generated with Claude Code
https://claude.ai/code/session_01VdRsGGg4YRG6E9fgLGbpdF