PR-readiness fixes for ChordalGMRF#1
Open
timweiland wants to merge 7 commits intosamuelsonric:mainfrom
Open
Conversation
- Replace the broken vendored deps/MooncakeSparse submodule with the registered MooncakeSparse 0.1 (Mooncake 0.5.25 compat now satisfiable by DifferentiationInterface 0.7.17). - Drop unused `chordal` and `triangular` imports from CliqueTrees.Multifrontal — `chordal` was removed in CliqueTrees 1.19.2 and neither symbol was referenced. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lost in 8e31503 (Zygote -> Mooncake) when src/piracy.jl was removed. Still needed for the Zygote-tagged GMRF constructor tests: ChainRulesCore's ProjectTo{SymTridiagonal} drops the factor of 2 from symmetry on the off-diagonal, and the explicit sum rrule sidesteps it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a docstring covering the role (Mooncake-friendly chordal Cholesky backing), fields, and construction modes. Include the type in docs/src/reference/gmrfs.md alongside GMRF. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The test asserts the NB→Poisson Hessian convergence at rtol=1e-6 with η = randn(5). Unlucky draws push exp(η) large enough that the 1/r correction at r=1e8 exceeds tolerance. Pin the seed so the assertion is reproducible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two issues prevented the chordal autodiff tests from running cleanly once the branch was rebased onto the workspace changes on main: 1. The logpdf rrule was registered for `::AbstractGMRF`, which catches `ChordalGMRF` and then crashes accessing `x.linsolve_cache.alg` inside the fallback error message. Restrict to `::GMRF` so ChordalGMRF falls through to native Mooncake AD (which already gives correct gradients via MooncakeSparse's mul!/dot rrules). 2. The chordal test file shared two top-level names with the non-chordal one: `backends` and `test_gauss_approx_pipeline`. Both files include into the same module, so the chordal redefinitions shadowed the non-chordal ones at runtime — meaning the Zygote/ ForwardDiff testsets in test_gaussian_approximation.jl ended up running the ChordalGMRF pipeline. Rename to `chordal_backends` and `test_gauss_approx_pipeline_chordal`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Turing 0.40 pins DynamicPPL ~0.37, which is incompatible with the Mooncake 0.5.25+ this PR requires. Allowing 0.41-0.44 lets the resolver land on Turing 0.44.4 + DynamicPPL 0.41.6, which compose with the new stack. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
timweiland
force-pushed
the
pr-81-chordal
branch
from
6403135 to
166d727
Compare
Author
|
@samuelsonric Sorry for the LLM blurb up top. With the new DI version, the dependency conflict I mentioned on your PR is gone and things work again. This PR here is just to clean some stuff up and get everything ready for a merge. Once you merge this PR here, we can merge your actual PR. |
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.
Companion to your PR timweiland#81 against
timweiland/GaussianMarkovRandomFields.jl. Branch is rebased on the current upstreammainand adds 8 commits that get the diff CI-green.Summary
deps/MooncakeSparsegit-submodule reference, depend on the registeredMooncakeSparse 0.1instead. The DI/Mooncake compat that was blocking is now satisfiable (DI 0.7.17 + Mooncake 0.5.27 + MooncakeSparse 0.1.0).chordalandtriangularimports fromCliqueTrees.Multifrontal(chordalwas removed in CliqueTrees 1.19.2; the others are referenced).sum(::SymTridiagonal)rrule that was lost whensrc/piracy.jlwas removed — still needed for the Zygote constructor tests because of theProjectTo{SymTridiagonal}factor-of-2 bug.backends→chordal_backendsandtest_gauss_approx_pipeline→test_gauss_approx_pipeline_chordalintest_gaussian_approximation_chordal.jlso the chordal test file stops shadowing the GMRF/Zygote one at module scope.logpdfrrule from::AbstractGMRFto::GMRFsoChordalGMRFfalls through to native Mooncake AD (which already gives correct gradients via MooncakeSparse'smul!/dotrrules) instead of crashing on a missinglinsolve_cachefield.NegativeBinomialPoisson-limit test (was flaky due to unseededrandn); apply runic to the 5 files CI flagged; add aChordalGMRFdocstring + reference-page entry; bump docsTuringcompat to0.40-0.44so the docs env can resolve under Mooncake 0.5.Verification
make docs: clean, exit 0 (only pre-existing cosmetic warnings).Test plan
Mooncake ChordalGMRF autodiff tests(20/20)Gaussian Approximation - ChordalGMRF(27/27)make docsbuilds without errors