Md/phys tests - #31
Conversation
…Fixed first layer skip (still remains in shooting method solvers.)
…distribution. Changed how spherical integration is handled.
…lts now also agree for load number.
…more useful when obliqua also models core). Replaced pinv in core boundary condition in relaxation solvers with proper nullspace.
…core option for now.
…re+surface compatibility check. Added analytical limit tests.
…ero forcing frequency.
There was a problem hiding this comment.
Pull request overview
This PR expands Obliqua’s solid/fluid tidal response validation and updates the 1D solid (and mush) solvers to improve stability/consistency (boundary conditions, sign conventions, scaling), while also adding documentation theming and CI-visible test badges.
Changes:
- Adds non-dimensional scaling and boundary-condition plumbing to
solid1d*solvers, plus a new equilibrium-tide relaxation model (solid1d_equil_relax). - Adds/updates tests to validate core/surface/A-matrix compatibility and homogeneous-sphere limits across models.
- Updates docs/README presentation (badges + Documenter light/dark themes) and CI to publish badge JSON to a gist.
Reviewed changes
Copilot reviewed 36 out of 41 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test/test.toml |
Adds core_props test configuration key. |
test/test_solid1d.jl |
Updates tests for new compute_M/compute_y signatures and complex core parameters. |
test/test_solid1d_relax.jl |
Adjusts relax tests for complex types and updated boundary-condition logic. |
test/test_solid1d_mush.jl |
Updates mush tests for new solver signatures and avoids r=0 in test grid. |
test/test_solid1d_mush_relax.jl |
Updates core parameter types to complex. |
test/test_solid1d_equil_relax.jl |
New test suite for the new equilibrium relaxation solver and analytical identities. |
test/test_common.jl |
Adds self-consistency tests for get_Ic, get_A, and surface BC interactions; updates precision handling. |
src/solid1d.jl |
Updates gravity computation, propagator scaling support, and compute_M/compute_y to use non-dimensional scaling and revised BCs. |
src/solid1d_relax.jl |
Updates core BC construction (nullspace-based), types, and boundary-condition ordering. |
src/solid1d_mush.jl |
Mirrors solid1d changes for mush case: scaling, BC updates, porous-layer handling changes. |
src/solid1d_mush_relax.jl |
Updates core BC construction (nullspace-based) and core parameter types. |
src/solid1d_equil_relax.jl |
Adds a new equilibrium-tide relaxation solver module (2×2 system). |
src/fluid0d.jl |
Renames doc/variables from k2_* to kn_* in the fluid 0D Love number routine. |
src/common.jl |
Adds shared get_surface_bc! and equilibrium-fluid get_A!/get_A; adjusts core solution conventions and heating map scaling. |
res/config/all_options.toml |
Documents new module option and adds core_props option. |
README.md |
Adds centered badges (license, codecov, test badges) and minor formatting tweaks. |
docs/src/troubleshooting.md |
Adjusts heading levels. |
docs/src/reference/liquid-phase.md |
Adjusts heading levels. |
docs/src/install.md |
Adjusts heading levels. |
docs/src/index.md |
Adds badges to docs landing page. |
docs/src/explanation/main_loop.md |
Adjusts heading levels. |
docs/src/development.md |
Adjusts heading levels and content formatting. |
docs/src/assets/style.scss |
Adds shared SCSS palette, typography, and font-face declarations. |
docs/src/assets/style.css |
Adds CSS overrides for badges/images and light/dark display toggles. |
docs/src/assets/lightdefs.scss |
Adds Documenter light-theme variable overrides and custom styling. |
docs/src/assets/darkdefs.scss |
Adds Documenter dark-theme overrides and custom CSS fixes (incl. admonitions). |
docs/src/assets/light.scss |
Generated/combined light theme SCSS used for compilation. |
docs/src/assets/dark.scss |
Generated/combined dark theme SCSS used for compilation. |
docs/src/assets/fonts/OFL-*.txt |
Adds font license texts. |
docs/Project.toml |
Adds DocumenterTools dependency for theme compilation. |
docs/make.jl |
Compiles light/dark themes during docs build and configures HTML assets. |
.github/workflows/runtests.yml |
Adds a step to publish test badge JSON to a gist on push. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 79.32% 82.62% +3.30%
==========================================
Files 13 14 +1
Lines 2796 3051 +255
==========================================
+ Hits 2218 2521 +303
+ Misses 578 530 -48 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
timlichtenberg
left a comment
There was a problem hiding this comment.
Great progress on the tests and the theming, this is a lot of work. A few questions on the solver changes before I can approve.
get_A! in common.jl still takes ω as a required argument, but the two -ω^2*ρ inertial terms are commented out, not removed. Was that on purpose? The new Ic vs A consistency test can't catch this either way, since get_Ic's "solid" branch has no ω term.
Zero forcing frequency now reroutes to solid1d-equil-relax whenever module_solid contains "solid1d", which also matches "solid1d-mush". Doesn't that send mush segments to the plain solid-body model at σ=0, dropping the mush arguments?
The old surface condition combined y6 + (n+1)/r*y5. The new get_surface_bc! only sets y6. Is that a basis change that makes the y5 term redundant, or did it get dropped?
get_core_bc! moved from pinv to nullspace, which is the headline boundary-condition fix, but the old unit test for it (checking B's shape and entries) was deleted with nothing in its place. Worth a direct test on the new implementation?
get_g divides by r^2, including at r=0. The mush test grid moved from r=0.0 to r=0.1 in this PR. Is that working around the same division rather than fixing it?
Small one: the docs build is failing in CI. DocumenterTools is in docs/Project.toml now, but documentation.yml never installs from that project file, so docs/make.jl's using DocumenterTools errors out.
There was a problem hiding this comment.
Summary
Awesome work, @MarijnJ0 - thanks for this.
The bugfix changes to the code seem reasonable to me, although some of the sections are still light on in-line comments, which makes it hard to understand all of the linear algebra that's implemented. I assume the (many) sign-changes you've included in this PR are some of the key bug fixes in the core model? I appreciate you adding more docstrings throughout the codebase, though.
The tests are great. They seem to include a range of unit, smoke, and integration tests. You've even included comparisons against 'classic' tidal models in the appropriate endmember regimes (e.g. fully liquid tides).
I ran the tests on my laptop (Fedora 44 on AMD, Julia 1.12.6) and they all pass fine. I am able to use the coverage script which 82% coverage. The docs also build without errors and render fine.
Changes required
-
The test count badges are created, although they are all labelled as unit tests. The integrate test badge shows zero. Is this correct?
-
The documentation shows the same badge three times. Are these meant to be the three test-type badges? The documentation workflow is also failing because of a missing
DocumenterToolspackage. -
The link to the AGNI documentation is broken in the Obliqua docs 'ecosystem' page.
-
The reference tables explaining config parameters should go into the Reference section of the docs, not within Tutorial 2.
-
The tutorials do not really feel like a tutorial still. They feel like a series of commands that the user runs without knowing what they do or achieve. Please at least include some figures to show what is outputted, and explain these in the tutorial pages.
-
The "How-To guides" page is empty and the new
solid1d_equil_relaxmodel is missing a documentation entry. -
Please add
coverage.*,src/*.cov, anddocs/build/to the.gitignore, to avoid uploading them to GitHub. -
See my additional in-line comments below.
|
Thanks for the feedback! To answer your questions:
This was a typo. Since the solid0d limit is only a valid approximation of solid1d when the inertial terms are removed from the motion matrix, I had commented them out during testing. I added the inertial_terms option to enable/disable inertial terms in motion matrix. This is useful for the shooting method, as it tends to diverge more quickly with these terms enabled. The relaxation based solver handles them no problem.
No, the solid1d_mush models are simply solid tide models that also capture the Darcy dissipation channel, which is only active in partial melts (mush). As such, using the solid1d_mush models requires the user to merge the solid and mush regimes down to some user defined viscosity. When using the relaxation based solver we can merge the entire solid and mush regimes. Regardless of the choice, the distinct tidal dissipation channels all turn off in the equilibrium limit, hence Im(k_n) = 0 by definition. The deformation is purely elastic, and the Re(k_n) depends merely on the mass distribution (i.e. the density profile). This means that solid1d_equil_relax is the correct simplified version of all other solid1d models. I suppose that if we drop the assumption of homogenity in the fluid regime, then the equations would also reduce to solid1d_equil_relax, Potentially we should apply solid1d_equil_relax in the entire mantle, but we would be mixing assumptions at this point, hence I only added it in place of the solid1d (inhomogeneous) models.
This is because of a mistake from before. I had originally confused two conventions. We should not include y5 in the surface boundary given our definition of y6 (see overleaf). This was introduced in the more recent works to simplify the surface boundary conditions. Additionally, I changed the normalization of the loading potential (G/G0 / R * U_prime). The latter does not apply to the equilibrium model, but since it relies on y7 = y6 + 4πG/g y2 it is always zero, no matter the normalization. With these changes, the boundary conditions now match those used in Sabadini 2016.
I added a test that checks if B is the nullspace through B * Ic = 0.
The solid1d models all have a singularity at r=0, not just due to the gravity calculation, but also in the motion matrices. Hence the analytically derived core solution vectors. They literally allow you to set r as small as you like > 0 (you dont have to use them to 'model' the entire metal core, we just do so because we dont have the inner core radial profiles in obliqua yet). So that, say the first layer is set to start at r = 0, then we must at least set r --> r = 0 + epsilon for some tiny epsilon. Since the first layer skip bugs got fixed I had to change the r=0 to r=0.1. In the main run_tides function I now added a check that sets r[1] --> 1e-6 * r[2] if r[1] = 0. As well as, several checks in the run_solid wrapper functions to catch this.
I added it to the build script, it should work now.
Yes, I did not add these tests yet. But copied over the badges etc, so that it will automatically grab these once they are added. Note, I changed the runtests.yml file temporarly, but will add back the if statement:
Yes they should be different: unit, integ, unit+integ.
I have changed it to the one used on the PROTEUS site: https://www.h-nicholls.space/AGNI/ (please lmk if this is incorrect.). Additionally, I changed the rest to the proteus-framework.org/ urls.
Moved the description to the reference, kept the read command in the tutorial.
This will take some time, I will try improving the docs over the next few days.
Added a new page for this.
Done. I will wait for the tests to finish, and see if the badges get updated... |
|
Docs preview at https://proteus-framework.org/Obliqua/previews/PR31/ The badges where created and look correct. I'll change the runtests.yml back now. |
nichollsh
left a comment
There was a problem hiding this comment.
Thanks for this Marijn. I love the animation on the homepage! Great work. However, I have a number of critical suggestions which need to be addressed before this is ready. See my in-line comments below and higher-level comments in this review.
This is my "human" review, but I'll get Claude to take a pass, too.
Tests
The tests pass on my laptop (Fedora 44 on x86, Julia 1.12.6). The coverage is 81.7% - which is very nice.
Tutorial 0D
I managed to get the tutorial working, but it doesn't work as-described in the docs. The final plot looks as below, which does indeed reproduce the one you've provided.
Tutorial 1D
Doesn't work because the file channel_showcase_updated.json is missing from the repo. I tried using the test_*.json files instead, but they fail with a KeyError on "phi".
Write NetCDF error
When writing the output netcdf files, I get the following error if the file already exists. Consider checking + warning + removing the file first, if it exists.
``log
[ Info: Writing results to /home/harrison/Projects/Obliqua/out/0_obliqua.nc
ERROR: NetCDF error: Permission denied (NetCDF error code: 13)
### Docs links
Many of the documentation links are broken. This is for various reasons including: incorrect HTML/Markdown syntax, linking directly to proteus-framework.org, typos, and some links are simply missing.
For example, the page `running_output.md` doesn't appear in the sidebar.
|
Selected comments from Claude code review, two things:
The 4x4 fluid-body to Unlike the other sign changes in this PR, this one isn't mentioned anywhere. It also has no callers anywhere in
When directly on its Suggest operating on a local copy of |
… obliqua/conda env to installation page.
|
Thanks Harrison! I have added the requested fixes to the docs. Regarding the code, the unused 4x4 A matrix was added mainly for testing purposes, but since it doesn't add much to the relaxation based solvers, I decided to remove it completely. Potentially, it can be reintroduced at some point in the future for completeness. The issue with the patch leaking into the heating profile is now also fixed. Before reassigning the first entry in the array, the code now first creates a local copy. I tried both tutorials and they seem to work, I added the new data file so this should also be fixed now. For plotting I kept the python notebook, as I think it is more convenient to work with than plots.jl. For this, there is now also a conda install section in the docs. If you have any further comments do let me know! |
nichollsh
left a comment
There was a problem hiding this comment.
Great - thank you, @MarijnJ0, for implementing my suggestions and fixes.
This seems to work as described on my laptop. The tests still pass with good code coverage.
The 1D tutorial now works, and I get the plot below, which matches the documentation.
I will mention that the example notebook contains some "empty" plots of heating vs lon/lat. This isn't mentioned in the tutorial at the moment, so I suppose they are remnants or intended for future expansion?
Happy for this to be merged. Well done!
|
This PR is currently blocked by @timlichtenberg's previous review requesting changes. I have given it a look, but @timlichtenberg needs to re-review or dismiss the old one before it can be merged. |
|
Thanks again, great to hear it is working! FYI: The empty plots for the heatmaps only get populated when |
…branches. Added tests to probe compatibility with A.
|
These two commits also add the relevant fixes for the I verified the mush model against the description in overleaf, and they match. The final part of the derivation is now in overleaf, and is more of a simplification step. Additionally, to fix the inertial core solution vector, I found a typo in the original reference: https://www.aanda.org/articles/aa/full_html/2021/06/aa39433-20/aa39433-20.html see inline comments. This now adds the fully general solid solution, verified against A through finite-difference checks. This simplifies in the fluid limit to the The core solution vectors relate roughly as follows:
|
timlichtenberg
left a comment
There was a problem hiding this comment.
This is much improved. The new validation layer is exactly the kind of check I wanted to see: the core basis vectors are now checked against the motion matrix by finite-difference residual and against the Love 1911 closed form, and the 1D solid modules are checked against the 0D homogeneous-sphere solution in the analytical limit. That moves the physics from self-consistent to validated against independent references.
I ran the full suite on md/phys_tests (Julia 1.12): 591 tests pass, 0 failures.
Good work improving the PR. Approving.
Description
This PR introduces new and improved tests, as well as several bug fixes. With these changes the solid1d* models now closely agree on tidal dissipation and Love numbers in the regime where they are stable. The test output is now visible in the docs and README through new badges. In addition to this, I have added the themes from AGNI into Obliqua, and made several changes to get the dark theme to work (it is still not perfect, but looks ok).
Specifically, this PR adds the following:
Still to add
I have already some updated plotting scripts, and will add them in the upcomming days. First I want to work on the orbit/PROTEUS part.
I want to add some additional tests for the mush models to further verify the code.
The inertial core solution vector needs to be further looked into, I have found a good source.
Let me know if I missed something! :)
Validation of changes
See the dissipation channel example plot in the overleaf document.
Checklist
Relevant people
@nichollsh @timlichtenberg