Clean up some diagnostics and ensure that diagnostics return AbstractOperations by navidcy · Pull Request #94 · NumericalEarth/NumericalEarth.jl

navidcy · 2026-03-04T05:03:25Z

Continuing #71

Also includes #99

glwagner · 2026-03-04T05:10:59Z

 function net_ocean_freshwater_flux(esm::EarthSystemModel; reference_salinity = 35)
    ρᵒᶜ = esm.interfaces.ocean_properties.reference_density
    S₀ = convert(typeof(ρᵒᶜ), reference_salinity)
-    net_ocean_frashwater_flux = - ρᵒᶜ / S₀ * net_ocean_salinity_flux(esm)
-    return Field(net_ocean_frashwater_flux)
+    return - ρᵒᶜ / S₀ * net_ocean_salinity_flux(esm)


this is wrong right? we are actually given the freshwater flux. using a reference salinity will introduce an error that will prevent us from closing a water budget?

also is the sign - because salt flux is negative freshwater flux?

The negative sign is exactly because of that.

comment could be apropos!

We don't use salt/salinity anymore to infer the freshwater fluxes but rather we read the freshwater fluxes from the coupler.

glwagner

good improvement, but I'm worried that providing temperature fluxes is not sustainable. I also worry about the approximatudes of a reference_salinity. Why don't we actually save the freshwater flux? This is hard to recover afterwards, eg if it comes from the atmosphere model's precipitation or land runoff

navidcy · 2026-03-04T05:16:09Z

good improvement, but I'm worried that providing temperature fluxes is not sustainable. I also worry about the approximatudes of a reference_salinity. Why don't we actually save the freshwater flux? This is hard to recover afterwards, eg if it comes from the atmosphere model's precipitation or land runoff

To be honest, regarding temp-heat dings: the important diagnostics are the heat fluxes! I'm very happy to drop the temperature fluxes; nobody ever uses/analyses/plots those!

glwagner · 2026-03-04T05:16:28Z

ok yeah i def think we should drop temperature fluxes!

codecov · 2026-03-04T05:21:54Z

Codecov Report

❌ Patch coverage is 56.66667% with 13 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/Diagnostics/interface_fluxes.jl	0.00%	10 Missing ⚠️
...dels/InterfaceComputations/sea_ice_ocean_fluxes.jl	90.90%	1 Missing ⚠️
...mputations/sea_ice_ocean_heat_flux_formulations.jl	0.00%	1 Missing ⚠️
src/Oceans/assemble_net_ocean_fluxes.jl	80.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

navidcy · 2026-03-04T05:25:18Z

ok yeah i def think we should drop temperature fluxes!

a9d7255

glwagner · 2026-03-04T05:27:02Z

 Return the sea ice-ocean salinity flux (g/kg m s⁻¹) at the sea ice-ocean interface
 in a coupled `esm`.
 """
 function sea_ice_ocean_salinity_flux(esm::EarthSystemModel)


It is worth noting that this flux actually has units g / kg * m / s. "salinity" is properly unitless, (or just 1/1000). there is another quantity, the "salt flux", which we think would probably have kg * m/s (eg a mass flux)

That's exactly what's mentioned in the docstring, right?

it doesnt say anything about the salt flux but you're right that it mentions the units

Perhaps when writing the docstrings I've used "salt flux" and "salinity flux" interchangeably.
Is there a difference between the two I should be aware of?

Your message suggests they are different. Salt flux is the mass of the salt, salinity is the "relative saltiness".

perhaps my head is spinning now and I'm not communicating clearly? I banged it the other day and today it decided to hurt...

but isn't that an approximation, and not the true salinity flux which uses the surface salinity (prognostic, depends on xy)

cc @taimoorsohail?

NumericalEarth.jl/src/Oceans/assemble_net_ocean_fluxes.jl

Line 163 in c22edb6

Jˢao = - Sᵒᶜ * ΣFao

NumericalEarth.jl/src/Oceans/assemble_net_ocean_fluxes.jl

Line 99 in c22edb6

Sᵒᶜ = ocean_surface_salinity[i, j, 1]

I see... so essentially J^S is the freshwater fluxes after multiplied by the surface salinity. So we should recover the freshwater fluxes by dividing by the actual surface salinity, not any reference salinity value, right?

That's what you are saying?

taimoorsohail · 2026-03-04T10:40:29Z

Yep agree to drop salt and temperature fluxes!

navidcy · 2026-03-04T10:42:59Z

Yep agree to drop salt and temperature fluxes!

done

* Modified freshwater flux definitions to match volume fluxes from interfaces * Update freshwater diagnostics docs and calls * Align freshwater diagnostics to mass flux units * Remove redundant sea-ice freshwater write in net ocean assembly * Apply suggestions from code review --------- Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>

navidcy · 2026-03-04T12:07:48Z

@glwagner you approved but lots has changed; could you have another look?

glwagner · 2026-03-04T17:49:44Z

+function atmosphere_ocean_freshwater_flux(esm::EarthSystemModel)
+    ρᵒᶜ = esm.interfaces.ocean_properties.reference_density
+    return ρᵒᶜ * esm.interfaces.atmosphere_ocean_interface.fluxes.freshwater_flux
+end


I guess there is a conflict here in terminology. probably the property should be changed to freshwater_volume_flux and we can define freshwater_flux as a mass flux.

Or we can rename all freshwater flux diagnostics to freshwater_mass_flux?

and you propose defining freshwater_flux as a volume flux?

esm.interfaces.atmosphere_ocean_interface.fluxes.freshwater_flux

is a volume flux, right?

We leave that as is, and the freshwater_flux diagnostics (which are mass fluxes) we add the _mass_ make it explicit that they differ from the

esm.interfaces.atmosphere_ocean_interface.fluxes.freshwater_flux

?

This is one option.

I am proposing a different path: define freshwater_flux as a mass flux, and rename the internal variable to make specific that it is a volume flux.

The reason to do this is that we really want to work with mass fluxes and a volume flux is not something user facing.

Gotcha! Deal

That's a big change, right? I mean, there are a lot of places where in the internals of the coupler there is freshwater_flux and all need to change to freshwater_volume_flux?

The same should happen in the ocean-sea ice coupler?

isn't it just a search and replace?

Sure, the workload is not my concern.
I just wanna confirm I understood correctly what I need to search and replace for.

Could you do it, perhaps?
I'm getting confused which variables are already in mass flux and which in volume flux....

simone-silvestri · 2026-03-13T09:26:27Z

-    return Field(sea_ice_ocean_freshwater_flux)
-end
+sea_ice_ocean_freshwater_flux(esm::EarthSystemModel) =
+    esm.interfaces.sea_ice_ocean_interface.fluxes.freshwater_flux


I don't think this exists. It's a salt flux, so we need to convert it to freshwater.

ah scratch it. I didn't see it is part of this PR to add it

navidcy · 2026-05-23T01:24:46Z

Too many merge conflicts; closing in favour of #281

diagnostics return AbstractOperations

f5e9464

navidcy requested a review from glwagner March 4, 2026 05:03

navidcy added the diagnostics 🩺 label Mar 4, 2026

glwagner reviewed Mar 4, 2026

View reviewed changes

Comment thread src/Diagnostics/interface_fluxes.jl Outdated

glwagner reviewed Mar 4, 2026

View reviewed changes

glwagner approved these changes Mar 4, 2026

View reviewed changes

drop temperature flux diags

a9d7255

glwagner reviewed Mar 4, 2026

View reviewed changes

navidcy added 2 commits March 4, 2026 11:36

drop salinity fluxes

e2d86ff

drop salinity fluxes..

c934dc9

navidcy changed the title ~~Diagnostics return AbstractOperations~~ Clean up some diagnostics and ensure that diagnostics return AbstractOperations Mar 4, 2026

Merge branch 'main' into ncc/diags-return-abstract-operations

bc2ecbc

taimoorsohail reviewed Mar 4, 2026

View reviewed changes

Comment thread src/Diagnostics/interface_fluxes.jl

taimoorsohail reviewed Mar 4, 2026

View reviewed changes

Comment thread src/Diagnostics/interface_fluxes.jl

taimoorsohail reviewed Mar 4, 2026

View reviewed changes

Comment thread src/Diagnostics/interface_fluxes.jl

cleanup

fe8bb33

taimoorsohail reviewed Mar 4, 2026

View reviewed changes

Comment thread src/Diagnostics/interface_fluxes.jl Outdated

navidcy and others added 6 commits March 4, 2026 11:49

pass reference_salinity further down

e08da88

ensure we test for reference salinity

e9b0a46

rephrase

ec708fd

Merge branch 'main' into ncc/diags-return-abstract-operations

43ecf6f

fix tests

e1b5fd3

navidcy requested a review from glwagner March 4, 2026 12:07

navidcy added 3 commits March 4, 2026 13:16

don't expose frazil_heat_flux to users

786456b

Merge branch 'main' into ncc/diags-return-abstract-operations

977c2ec

code alignment

ccedd01

navidcy commented Mar 4, 2026

View reviewed changes

Comment thread src/EarthSystemModels/InterfaceComputations/component_interfaces.jl Outdated

Apply suggestions from code review

9dc7ac9

glwagner reviewed Mar 4, 2026

View reviewed changes

taimoorsohail added 2 commits March 5, 2026 09:55

Merge branch 'main' into ncc/diags-return-abstract-operations

f6c1fd2

Merge branch 'main' into ncc/diags-return-abstract-operations

42220a3

simone-silvestri reviewed Mar 13, 2026

View reviewed changes

merge main

0fddfcc

navidcy mentioned this pull request May 23, 2026

Clean up some diagnostics and ensure that diagnostics return AbstractOperations (take 2) #281

Open

navidcy closed this May 23, 2026

Conversation

navidcy commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glwagner left a comment

Choose a reason for hiding this comment

Uh oh!

navidcy commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glwagner commented Mar 4, 2026

Uh oh!

codecov Bot commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

navidcy commented Mar 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navidcy Mar 4, 2026 • 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!

taimoorsohail commented Mar 4, 2026

Uh oh!

navidcy commented Mar 4, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

navidcy commented Mar 4, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navidcy Mar 4, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navidcy commented Mar 4, 2026 •

edited

Loading

navidcy commented Mar 4, 2026 •

edited

Loading

codecov Bot commented Mar 4, 2026 •

edited

Loading

navidcy Mar 4, 2026 •

edited

Loading

navidcy Mar 4, 2026 •

edited

Loading

navidcy Mar 16, 2026 •

edited

Loading