Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix BT solver wide halo log_param and hifreq diagnostics timestamps #209

Merged
merged 4 commits into from
Sep 21, 2022

Conversation

herrwang0
Copy link

The first commit fixes a couple of typos in BT solver.

The second commit moves the log_param calls of the wide halo size parameters after the clone_MOM_domain call, where these parameters may be modified. Otherwise, the entries in MOM_parameter_doc.layout record only the use specified values or the default, which is usually zero. As a result, this commit may change MOM_parameter_doc.layout from the old runs.

The third commit addresses a potential problem with the timestamps of the BT step diagnostic outputs. Previously, the (hifreq) output between BT steps has a time interval = dtbt. While this is technically the right choice, it ignores the filter steps. As a result, the time interval at each baroclinic time step is not enough for all output from nstep+nfilter BT step. The filter step output has a timestamp that exceeds the end time of the current baroclinic step. The output at the filter steps then is never saved (overwritten by the output from the next baroclinic time step with the same timestamps), except for the last baroclinic timestep of the run.
This commit provides a walk-around. A nominal BT step size dtbt_diag is introduced to "squeeze" all nstep+nfilter output into the same baroclinic step time interval.

* Description of wt_eta related  variables
* Longname of PFu_BT
Logging BT halo size parameters should happen after the modification in
clone_MOM_domain, where they are lower-bounded by the domain halo.
This commit fixes this and therefore may result a change of parameters
in MOM_parameter_doc.layout in past runs.
This commit fixes the conflicting timestamps for diagnostics at the BT
step level (do_hifreq_output=True).
Instead of the actual dtbt, a nominal time interval is used to "squeeze"
output at all nstep+nfilter steps into a time interval of a single
baroclinic timestep.
@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Merging #209 (4ea3039) into dev/gfdl (fae71c4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 4ea3039 differs from pull request most recent head 670a81f. Consider uploading reports for the commit 670a81f to get more accurate results

@@            Coverage Diff            @@
##           dev/gfdl     #209   +/-   ##
=========================================
  Coverage     37.15%   37.15%           
=========================================
  Files           262      262           
  Lines         72744    72745    +1     
  Branches      13597    13597           
=========================================
+ Hits          27031    27032    +1     
  Misses        40699    40699           
  Partials       5014     5014           
Impacted Files Coverage Δ
src/core/MOM_barotropic.F90 58.74% <100.00%> (+0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

The compression of the times for the high frequency output so that all of the substeps, including those in the extra filter time, strikes me as a very clever idea. The other changes in this PR are clearly correcting minor issues with this file.

@marshallward
Copy link
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/16833 ✔️ 🟡

@marshallward marshallward merged commit d88c58a into NOAA-GFDL:dev/gfdl Sep 21, 2022
@herrwang0 herrwang0 deleted the bt_hifreq_diag branch September 10, 2024 21:37
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.

3 participants