Generalize `VarDims` to `VarLocation` and make domains explicit in all `variables` definitions by bgroenks96 · Pull Request #194 · NumericalEarth/Terrarium.jl

bgroenks96 · 2026-09-20T14:56:14Z

This PR is a continuation of #193 that makes a few wide reaching changes to the variable system to accommodate the new spatial domains defined by LandGrid:

Adds VarDomain with four subtypes: Ground, Snow, Canopy, and Surface, the latter of which supports 2D surface/skin variables only and does not correspond to a domain in LandGrid
Adds VarLocation which consists of both a VarDims (XY or XYZ) and a VarDomain
Allows variables to be defined at fixed points along an axis (typically the vertical axis) via Coordinate; concrete use cases are Top and Bottom which allow fluxes to be defined at the top Face or Center of a grid axis. The resulting Field is constructed with indices set accordingly. This differs from a 2D field with Nothing location in that Oceananigans treats the Field as having a specific location on the axis rather than having no location at all (see discussion in Issues with bloated model state and long compile times #66)
As a consequence of the above, 2D Top or Bottom located Fields are now indexed in kernels with i, j, end rather than i, j, 1

Closes #178
Closes #182

olivierbonte

Think in general that his a nice addition. Main omments:

Ideally (low prioririty), you would also have the option to not be forced to use a variable domain (e.g. for simple models)
Surface is always a bit of an ambiguous term (top of soil/snow vs. interface). Here goal is for it to function as interface, but this can become a bit uncelar for some configurations( e.g. canopy air space as interface, or canopy temperature being used as surface temperature so then not belonging to the canopy anymore if you set it as Surface)
Would be nice to have separate domain for atmospheric inputs

bgroenks96 · 2026-09-25T07:39:34Z

@maximilian-gelbrecht I may need your help with this Reactant test failure. None of my fixes so far work.

olivierbonte

Only some minor comments on domain choices. I am not that familiar with the interals, but all changes seem reasonable / logic to my limited understanding.

Replace literal `[i, j, 1]` vertical indices with `[i, j, end]` throughout the kernel functions. A variable declared at the top or bottom of a domain is stored at that interface rather than at `k = 1`, so `end` is the index which resolves correctly for every slab field: `XY` and `Bottom` fields give 1, a `Top` field at `Center` gives `Nz`, and a `Top` field at `Face` gives `Nz + 1`.

Move fluxes and other quantities which exist at a single interface from the domain's bare `XY()` dimensions to `Top()` or `Bottom()`, so that the declaration states where the variable lives rather than leaving it implicit. Stepping a prognostic declared at an interface also required a fix to `explicit_step!`, which dispatched on the vertical location parameter alone.

@ref

`XY` and `XYZ` lost their docstrings when they were converted from `@kwdef` structs to type aliases, which left every existing `[`XY`](@ref)`/`[`XYZ`](@ref)` in the documentation unresolvable. `Top`, `Bottom`, and the four `VarDomain` markers were not previously documented.

Hopefully fixes a ReactantError arising from `apply_type_with_promotion`

EnzymeAD/Reactant.jl#3326

Co-authored-by: Olivier Bonte <65309133+olivierbonte@users.noreply.github.com>

bgroenks96 · 2026-09-25T14:54:31Z

The reactant errors are fixed but at the cost of loosening type bounds in 2d832d4 which I am not happy about. This change should be reverted when the upstream bug is fixed.

olivierbonte

For me all looks good now!

bgroenks96 · 2026-09-25T16:34:45Z

I will wait for @maximilian-gelbrecht to comment on this before merging.

maximilian-gelbrecht

I like this. I just think that it needs to be documented a bit better in the main docs and examples.

The Reactant issue has been fixed and a new released will probably issued within the next days. Do you want to wait for this or adjust in a follow up?

bgroenks96 · 2026-10-05T15:48:39Z

Do you want to wait for this or adjust in a follow up?

I would look at it again in a follow-up PR.

bgroenks96 · 2026-10-05T16:39:06Z

It seems that the Reactant "fix" actually broke our build again...

Either that or the merge broke something. I'll have Claude investigate.

bgroenks96 requested review from maximilian-gelbrecht and olivierbonte September 20, 2026 14:56

bgroenks96 added this pull request to stack #195 September 20, 2026 14:56

olivierbonte reviewed Sep 22, 2026

View reviewed changes

bgroenks96 force-pushed the bg/var-domains branch 2 times, most recently from 106a8b9 to 3cc4cad Compare September 24, 2026 13:52

bgroenks96 marked this pull request as ready for review September 24, 2026 14:09

Base automatically changed from bg/grid-refactoring to main September 25, 2026 09:27

bgroenks96 force-pushed the bg/var-domains branch from 74d0e61 to e7b4cbe Compare September 25, 2026 09:27

bgroenks96 mentioned this pull request Sep 25, 2026

Bug in apply_type_with_promotion causes BoundsError for certain type layouts EnzymeAD/Reactant.jl#3326

Closed

olivierbonte reviewed Sep 25, 2026

View reviewed changes

bgroenks96 added 18 commits September 25, 2026 15:09

Update imports and rename asset indices method

499b6c6

Add VarDomain and VarLocation for specifying var domain + dims

f08d6fc

Update all variable defs to include domain tags

eb6a76c

Update doc pages

199b4d7

Minor doc change

daca870

Fix formatting error

f72fffb

Fix incorrect domain markers

9da8e2a

Change runoff variables to ground-top rather than surface

f6d3aaf

Add atmosphere domain and move types to seaprate file

a1b2ad5

Make var domains optional update definitions

8f0d525

Fix reactant type bug

d1a6925

Fix outdated name in Reactant test

c732d0c

Add missing type arg to ReactantGrid

88aa095

Loosen type bounds on AbstractModel

521748f

Hopefully fixes a ReactantError arising from `apply_type_with_promotion`

Fix bug in linear ode example script

3c9749b

bgroenks96 added 2 commits September 25, 2026 15:10

Fix incorrect 2D indexing in ddm example

4110f0a

Temporarily remove type bounds to workaround Reactant bug

2d832d4

EnzymeAD/Reactant.jl#3326

bgroenks96 force-pushed the bg/var-domains branch from 5b8bda6 to 2d832d4 Compare September 25, 2026 13:11

bgroenks96 and others added 6 commits September 25, 2026 15:47

Revert previous change to example script

1f3d1f3

Update docs/src/running/input_sources.md

36bdb24

Co-authored-by: Olivier Bonte <65309133+olivierbonte@users.noreply.github.com>

Remove unncessary Ground domain markers

0b2b3fa

Add domain option for RingGrids -> Field converters

5bd5684

Change snow_fall domain to atmosphere in example

c5698eb

Change rainfall_ground to Ground(Top())

3067294

olivierbonte approved these changes Sep 25, 2026

View reviewed changes

maximilian-gelbrecht reviewed Sep 29, 2026

View reviewed changes

Comment thread docs/src/extending/coupling_processes.md

Comment thread examples/extending/linear_heat_conduction.jl

Comment thread examples/extending/simple_snow_ddm.jl Outdated

Comment thread src/diagnostics/surface_fluxes.jl

Comment thread src/domains.jl

olivierbonte mentioned this pull request Sep 29, 2026

Add plan (documentation only) force-restore 3-source hydrology model #201

Open

maximilian-gelbrecht mentioned this pull request Sep 29, 2026

Fix CI documentation #206

Merged

Merge branch 'main' into bg/var-domains

6d89709

bgroenks96 force-pushed the bg/var-domains branch from aaef46b to 4ede916 Compare October 5, 2026 15:59

bgroenks96 added 2 commits October 5, 2026 18:03

Improve documentation of variable dims/domains

07204a6

Add missing kwargs from merge

4a64b63

bgroenks96 force-pushed the bg/var-domains branch from 4ede916 to 4a64b63 Compare October 5, 2026 16:04

maximilian-gelbrecht reviewed Oct 5, 2026

View reviewed changes

Comment thread docs/src/extending/state_variables.md

maximilian-gelbrecht approved these changes Oct 5, 2026

View reviewed changes

Remove var domains from snow ddm example

e2b31e3

Remove NF bounds on grid and timestepper in veg model

8813eb2

bgroenks96 force-pushed the bg/var-domains branch from 16b35bf to 8813eb2 Compare October 5, 2026 18:50

bgroenks96 merged commit ed652ed into main Oct 5, 2026
10 checks passed

Conversation

bgroenks96 commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olivierbonte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bgroenks96 commented Sep 25, 2026

Uh oh!

olivierbonte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bgroenks96 commented Sep 25, 2026

Uh oh!

olivierbonte left a comment

Choose a reason for hiding this comment

Uh oh!

bgroenks96 commented Sep 25, 2026

Uh oh!

maximilian-gelbrecht left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bgroenks96 commented Oct 5, 2026

Uh oh!

Uh oh!

bgroenks96 commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bgroenks96 commented Sep 20, 2026 •

edited

Loading

bgroenks96 commented Oct 5, 2026 •

edited

Loading