Skip to content

Use SST and SSS consistent with MARBL tracers#374

Merged
alperaltuntas merged 4 commits into
NCAR:dev/ncarfrom
mnlevy1981:marbl_consistent_sst_and_sss
Aug 26, 2025
Merged

Use SST and SSS consistent with MARBL tracers#374
alperaltuntas merged 4 commits into
NCAR:dev/ncarfrom
mnlevy1981:marbl_consistent_sst_and_sss

Conversation

@mnlevy1981
Copy link
Copy Markdown
Collaborator

When computing surface fluxes and interior tendencies for MARBL tracers, we want to use T & S from before they are modified by tracer_vertdiff(). We do this by adding prediabatic_T and prediabatic_S to the diabatic control structure, and then passing these values instead of tv%T and tv%S to MARBL.

The existing MARBL driver uses T & S from the end of the diabatic routine as
inputs into surface_flux_compute() and interior_tendency_compute(). For
surface_flux_compute(), there is a discrepancy because we have not yet called
tracer_vertdiff for the MARBL tracers but have for T & S. To enforce
consistency among all the tracers, we want to use the pre-vertdiff T&S
quantities.

There is a little bit of infrastructure change -- I added 3D fields for T & S
at the beginning of the diabatic subroutine to the diabatic control structure
(we will eventually want to call interior_tendency_compute() before
tracer_vertdiff as well), and then had to add a routine to
MOM_tracer_flow_control.F90 to extract use_MARBL_tracers from the tracer flow
control structure. prediabatic_T and prediabatic_S are now optional arguments
to call_tracer_column_fns(), but that function will abort if USE_MARBL_TRACERS
is true and the arguments are not present because they are required in
MARBL_tracers_column_physics()
Fix failing doxygen / style check texts. Also add a long comment to
diabatic_ALE explaining why we use prediabatic_T and prediabatic_S (maybe the
comment belongs in diabatic_init?)
@mnlevy1981
Copy link
Copy Markdown
Collaborator Author

As a first pass, I only used prediabatic_T and prediabatic_S for the surface flux computation; I will mark this as "ready for review" when we are also using these arrays to compute the interior tendency (which will require reordering some calls in MARBL_tracers_column_physics())

Reordered MARBL_tracers_column_physics so that surface_flux_compute() and
interior_tendency_compute() are both called before tracer_vertdiff. Now use
prediabatic T & S for interior tendency computation, and also use h_old as dz.

Note that this greatly increases the number of warnings from the co2calc
routine, and we are not sure why.
@mnlevy1981 mnlevy1981 marked this pull request as ready for review July 24, 2025 17:28
@mnlevy1981
Copy link
Copy Markdown
Collaborator Author

@gustavo-marques is using these changes in his latest runs where we will look at the effect of the two different vertical grids on BGC variables; assuming those runs don't raise any red flags, this PR is ready to go (it will also require marbl-ecosys/MARBL#480)

Comment thread src/tracer/MOM_tracer_flow_control.F90 Outdated
Comment thread src/tracer/MOM_tracer_flow_control.F90 Outdated
Comment thread src/tracer/MOM_tracer_flow_control.F90
1. removed spaces from "end if" to match style guide recommendations
2. cleaned up comment in extract_tracer_flow_member (refer to
   tracer_flow_control_CS rather than diabatic_CS)
3. Consistent capitalization of MARBL in use_MARBL_tracers (and use_MARBL)
   throughout the codebase
Copy link
Copy Markdown
Collaborator

@klindsay28 klindsay28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alperaltuntas
Copy link
Copy Markdown
Member

@mnlevy1981 Has this PR been tested, or is it ready for being tested?

@mnlevy1981
Copy link
Copy Markdown
Collaborator Author

mnlevy1981 commented Aug 21, 2025

@alperaltuntas it is ready for testing (I'm surprised I don't have a comment mentioning test results -- I'm pretty sure I ran at least aux_mom_MARBL but I can't find any record of it)

note that this is answer-changing for cases with USE_MARBL_TRACERS=TRUE

Copy link
Copy Markdown
Collaborator

@gustavo-marques gustavo-marques left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@alperaltuntas
Copy link
Copy Markdown
Member

@mnlevy1981 For both MARBL test cases in aux_mom, I got ~35% performance hit. Is this expected?

    FAIL SMS.TL319_t232.G1850MARBL_JRA.derecho_intel TPUTCOMP Error: TPUTCOMP: Throughput changed by 35.14%: baseline=4.963 sypd, tolerance=25%, current=3.219 sypd

@mnlevy1981
Copy link
Copy Markdown
Collaborator Author

@mnlevy1981 For both MARBL test cases in aux_mom, I got ~35% performance hit. Is this expected?

    FAIL SMS.TL319_t232.G1850MARBL_JRA.derecho_intel TPUTCOMP Error: TPUTCOMP: Throughput changed by 35.14%: baseline=4.963 sypd, tolerance=25%, current=3.219 sypd

Can you try updating MARBL to the head of the development branch? These changes greatly increased the number of MARBL warnings that were being written to cesm.log, and marbl-ecosys/MARBL#480 brought that warning count back down to a reasonable number. I'm a little surprised it was a 35% hit, but I want to make a new MARBL tag for MOM-interface with these changes anyway.

@alperaltuntas
Copy link
Copy Markdown
Member

@mnlevy1981 For both MARBL test cases in aux_mom, I got ~35% performance hit. Is this expected?

    FAIL SMS.TL319_t232.G1850MARBL_JRA.derecho_intel TPUTCOMP Error: TPUTCOMP: Throughput changed by 35.14%: baseline=4.963 sypd, tolerance=25%, current=3.219 sypd

Can you try updating MARBL to the head of the development branch? These changes greatly increased the number of MARBL warnings that were being written to cesm.log, and marbl-ecosys/MARBL#480 brought that warning count back down to a reasonable number. I'm a little surprised it was a 35% hit, but I want to make a new MARBL tag for MOM-interface with these changes anyway.

Updating MARBL to the head of development brought throughput to 4.77.

@alperaltuntas alperaltuntas merged commit 5802794 into NCAR:dev/ncar Aug 26, 2025
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants