Add tests for `set!` #163

glwagner · 2025-11-13T16:49:37Z

I am expecting the GPU+Float32 to fail here, but not sure.

giordano · 2025-11-13T17:23:51Z

Someone broke isapprox on Fields, I wonder who could have been 😩

glwagner · 2025-11-13T17:45:06Z

Someone broke isapprox on Fields, I wonder who could have been 😩

ah no its not your fault! it looks like it is not defined for array/scalar comparison, eg

julia> a = ones(10, 10)
10×10 Matrix{Float64}:
 1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0
 1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0
 1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0
 1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0
 1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0
 1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0
 1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0
 1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0
 1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0
 1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0  1.0

julia> a ≈ 1
ERROR: MethodError: no method matching isapprox(::Matrix{Float64}, ::Int64)
The function `isapprox` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  isapprox(::Missing, ::Any; kwargs...)
   @ Base missing.jl:90
  isapprox(::Integer, ::Integer; atol, rtol, nans, norm)
   @ Base floatfuncs.jl:229
  isapprox(::Number, ::Number; atol, rtol, nans, norm)
   @ Base floatfuncs.jl:220
  ...

Stacktrace:
 [1] top-level scope
   @ REPL[37]:1

glwagner · 2025-11-13T17:45:51Z

We do have an abstraction called ConstantField(c) that might work here, I will try it.

giordano · 2025-11-13T17:47:19Z

all(isapprox(1), c)? But it requires scalar indexing.

glwagner · 2025-11-13T17:48:40Z

all(isapprox(1), c)? But it requires scalar indexing.

We have an abstraction that represents a constant broadcast to an array (but without allocating memory for it). I guess this doesn't exist in julia base (or maybe it does somehow) but we can use it here, its called ConstantField

glwagner · 2025-11-13T17:51:17Z

We also need scalar indexing for ordinary approx right?

Or maybe I am confused. Do we hit the same codepath into GPUArrays with Field magically?

glwagner · 2025-11-13T17:52:59Z

Well, this works

julia> grid = RectilinearGrid(CPU(); size=(8, 8, 8), x=(0, 1_000), y=(0, 1_000), z=(0, 1_000))
8×8×8 RectilinearGrid{Float64, Periodic, Periodic, Bounded} on CPU with 3×3×3 halo
├── Periodic x ∈ [0.0, 1000.0) regularly spaced with Δx=125.0
├── Periodic y ∈ [0.0, 1000.0) regularly spaced with Δy=125.0
└── Bounded  z ∈ [0.0, 1000.0] regularly spaced with Δz=125.0

julia> c = CenterField(grid)
8×8×8 Field{Center, Center, Center} on RectilinearGrid on CPU
├── grid: 8×8×8 RectilinearGrid{Float64, Periodic, Periodic, Bounded} on CPU with 3×3×3 halo
├── boundary conditions: FieldBoundaryConditions
│   └── west: Periodic, east: Periodic, south: Periodic, north: Periodic, bottom: ZeroFlux, top: ZeroFlux, immersed: Nothing
└── data: 14×14×14 OffsetArray(::Array{Float64, 3}, -2:11, -2:11, -2:11) with eltype Float64 with indices -2:11×-2:11×-2:11
    └── max=0.0, min=0.0, mean=0.0

julia> c ≈ Field{Nothing, Nothing, Nothing}(grid)
true

glwagner · 2025-11-13T17:53:28Z

ConstantField does not work because it does not have a grid and therefore no notion of size. That is pretty easily changed though

giordano · 2025-11-13T17:53:53Z

We also need scalar indexing for ordinary approx right?

Not after CliMA/Oceananigans.jl#4924 🙂 (well, there's a call to norm which internally does a scalar indexing, but it's on a one-element array after a reduction, not on the whole field)

glwagner · 2025-11-13T17:56:50Z

We also need scalar indexing for ordinary approx right?

Not after CliMA/Oceananigans.jl#4924 🙂 (well, there's a call to norm which internally does a scalar indexing, but it's on a one-element array after a reduction, not on the whole field)

ok so we also need to remove all the allowscalar!

giordano · 2025-11-13T17:58:05Z

ok so we also need to remove all the allowscalar!

How was it? Great minds...? 5a8233d

glwagner · 2025-11-13T18:03:28Z

ok so we also need to remove all the allowscalar!

How was it? Great minds...? 5a8233d

😂

…ze.jl into glw/test-set

giordano · 2025-11-29T17:04:54Z

src/Microphysics/saturation_adjustment.jl

-abstract type AbstractEquilibrium end
-


I'm assuming that this wasn't meant to be removed: it disappeared in a merge commit, which feels unlikely to be done on purpose? Edit: tentatively changed in 53c7a56

giordano · 2025-11-29T17:19:27Z

src/AtmosphereModels/set_atmosphere_model.jl

-    g = thermo.gravitational_acceleration
-    z = znode(i, j, k, grid, c, c, c)


These lines also got removed in another merged commit: 4e642d1. Was that done on purpose? Edit: tentatively changed in 53c7a56

giordano · 2025-11-29T17:55:58Z

src/Microphysics/saturation_adjustment.jl

-@inline function adjust_state(𝒰₀, T, thermo, equilibrium)
+@inline function adjust_state(𝒰₀::AbstractThermodynamicState{FT}, T::FT,
+                              thermo::ThermodynamicConstants{FT},
+                              equilibrium::MixedPhaseEquilibrium{FT}) where FT


This stricter signature caused

MethodError: no method matching adjust_state(::Breeze.Thermodynamics.PotentialTemperatureState{Float64}, ::Float64, ::ThermodynamicConstants{Float64, CondensedPhase{Float64}, CondensedPhase{Float64}}, ::WarmPhaseEquilibrium) The function `adjust_state` exists, but no method is defined for this combination of argument types. Closest candidates are: adjust_state(::Breeze.Thermodynamics.AbstractThermodynamicState{FT}, ::FT, ::ThermodynamicConstants{FT}, !Matched::MixedPhaseEquilibrium{FT}) where FT @ Breeze ~/work/Breeze.jl/Breeze.jl/src/Microphysics/saturation_adjustment.jl:200

Do we need to define a different method for WarmPhaseEquilibrium? Or the last argument can be AbstractEquilibrium?

diff --git a/src/Microphysics/saturation_adjustment.jl b/src/Microphysics/saturation_adjustment.jl index 54bd973..393258f 100644 --- a/src/Microphysics/saturation_adjustment.jl +++ b/src/Microphysics/saturation_adjustment.jl @@ -199,7 +199,7 @@ end @inline function adjust_state(𝒰₀::AbstractThermodynamicState{FT}, T::FT, thermo::ThermodynamicConstants{FT}, - equilibrium::MixedPhaseEquilibrium{FT}) where FT + equilibrium::AbstractEquilibrium) where FT pᵣ = 𝒰₀.reference_pressure qᵗ = total_specific_moisture(𝒰₀) @@ -210,7 +210,7 @@ end @inline function saturation_adjustment_residual(T::FT, 𝒰₀::AbstractThermodynamicState{FT}, thermo::ThermodynamicConstants{FT}, - equilibrium::MixedPhaseEquilibrium{FT}) where FT + equilibrium::AbstractEquilibrium) where FT 𝒰₁ = adjust_state(𝒰₀, T, thermo, equilibrium) T₁ = temperature(𝒰₁, thermo)

would fix that issue and make all the saturation_adjustment tests pass, but I'm not sure this is the intended design, so I'm leaving that to you.

I tentatively pushed 705c0b9 to implement the above, let me know if you didn't want it.

giordano · 2025-11-29T18:05:35Z

src/Microphysics/saturation_adjustment.jl

 """
 struct WarmPhaseEquilibrium <: AbstractEquilibrium end
 @inline equilibrated_surface(::WarmPhaseEquilibrium, T) = PlanarLiquidSurface()
+convert_equilibrium(FT, ::WarmPhaseEquilibrium) = WarmPhaseEquilibrium()


Is this method necessary? I presume this is already covered by

Breeze.jl/src/Microphysics/saturation_adjustment.jl

Line 63 in aa22c7b

convert_equilibrium(FT, equil) = equil # fallback

?

…ent_residual` Accept `AbstractEquilibrium` instead of `MixedPhaseEquilibrium{FT}`.

giordano · 2025-12-03T01:16:11Z

I should have resolved all merge conflicts, but now there are some tests failures in atmosphere_model_unit.

This PR has had a few conflicts with other PRs over the last few days, it'd be good to get this in soon if it's still desired, otherwise it'll keep conflicting and breaking forever. Edit: the failures in atmosphere_model_unit were already in aa22c7b, at least I didn't break anything today!

glwagner added 2 commits November 13, 2025 09:49

Add tests for set!

4392c8f

I am expecting the GPU+Float32 to fail here, but not sure.

Add microphysics parameter to atmosphere model tests

6f0d279

Refactor tests to use constant_field function

ff03e99

glwagner and others added 3 commits November 13, 2025 13:27

rm allowscalar

2ab21aa

add allowscalar back

1f06409

Merge branch 'main' into glw/test-set

05f4d4e

navidcy added the testing 🧪 label Nov 13, 2025

navidcy changed the title ~~Add tests for set!~~ Add tests for set! Nov 13, 2025

glwagner and others added 7 commits November 13, 2025 13:54

better show for AnelasticFormulation

b2cdea9

rearrange kernel func

4399ca2

diagonalize some routines by floating point

df7d350

Merge branch 'glw/test-set' of https://github.com/NumericalEarth/Bree…

af52101

…ze.jl into glw/test-set

Merge branch 'main' into glw/test-set

8800359

Merge branch 'main' into glw/test-set

4e642d1

Merge branch 'main' into glw/test-set

83037e0

giordano reviewed Nov 29, 2025

View reviewed changes

Tentative fix of broken merge resolutions

53c7a56

giordano added 2 commits November 29, 2025 17:33

Remove stale import

45d9731

specific_moisture -> moisture_mass_fraction

aa22c7b

giordano reviewed Nov 29, 2025

View reviewed changes

giordano added 4 commits December 3, 2025 00:52

Merge remote-tracking branch 'origin/main' into glw/test-set

b584a84

Tentatively relax signature of adjust_state and `saturation_adjustm…

705c0b9

…ent_residual` Accept `AbstractEquilibrium` instead of `MixedPhaseEquilibrium{FT}`.

Fix type parameters

31ffddf

Remove redundant @allowscalars

6d91ea2

		g = thermo.gravitational_acceleration
		z = znode(i, j, k, grid, c, c, c)

Add tests for set! #163

Are you sure you want to change the base?

Add tests for set! #163

Uh oh!

Conversation

glwagner commented Nov 13, 2025

Uh oh!

giordano commented Nov 13, 2025

Uh oh!

glwagner commented Nov 13, 2025

Uh oh!

glwagner commented Nov 13, 2025

Uh oh!

giordano commented Nov 13, 2025

Uh oh!

glwagner commented Nov 13, 2025

Uh oh!

glwagner commented Nov 13, 2025

Uh oh!

glwagner commented Nov 13, 2025

Uh oh!

glwagner commented Nov 13, 2025

Uh oh!

giordano commented Nov 13, 2025

Uh oh!

glwagner commented Nov 13, 2025

Uh oh!

giordano commented Nov 13, 2025

Uh oh!

glwagner commented Nov 13, 2025

Uh oh!

giordano Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giordano Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giordano Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

giordano Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

giordano Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

giordano Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

giordano commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add tests for `set!` #163

Add tests for `set!` #163

giordano Nov 29, 2025 •

edited

Loading

giordano Nov 29, 2025 •

edited

Loading

giordano commented Dec 3, 2025 •

edited

Loading