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

Diag scaling p2 #1002

Merged
merged 6 commits into from
Sep 19, 2019
Merged

Diag scaling p2 #1002

merged 6 commits into from
Sep 19, 2019

Conversation

marshallward
Copy link
Collaborator

This PR fixes all of the diagnostic scaling issues in the current test cases (TC0-3).

Most consist of supplying the dimension conversions in register_diag_field, along with a few genuine fixes.

The main part worth mentioning is the fixes associated with tracer diagnostics. While all are now working, they were fixed used the flux_scale and conv_scale arguments in various places. In just about every case, it was following precedent in adjacent code. But it still seems that this isn't the ideal use of these arguments, which were defined for more specific purposes. For now, I think it's fine, but we may want to revisit this in the future.

The PR does not fix the "negative zero" issues in one of the barotropic transport diagnostics, so we cannot yet enable the diagnostic checksum support in the test suite.

The horizontal_average_diag_field function uses thickness when computing
layer averages in some cases.  Reproducibility requires that
reproducible sums be calculated in MKS units.  While the grids have been
converted to MKS lengths, the thickeness have not.

This patch rescales the thicknesses to volumes, which restores the
reproducibility of the dimentionally scaled quantities involving H.
This fixes up the dimensional scaling of several diagnostics:

* dynamics_h_tendency
* MLD_Restrat
* ea, eb
* h_predia
* frazil_h

We also include a change to the flux_to_kg_per_s scaling factor.
Previously, this was dynamically set the flux units to either m3 s-1 or
kg s-1 depending on the Boussinesq mode.  However, the units of the
diagnostics using this factor (uhml, vhml) were always explicitly set to
kg -1.  Assuming that the scaled fluxes were H L2 T-1, this would have
given the wrong scaling in non-Boussinesq mode.

We resolve this by removing the conditional and always using
GV%H_to_kg_m2 when defining this scaling factor.
This patch fixes the H dimensional scaling of several diagnostics.

* h_preale
* h_predia
* Tflx_dia_diff
* Tflx_dia_adv
* Sflx_dia_diff
* Sflx_dia_adv
* diabatic_diff_h
* boundary_forcing_h
* frazil_h
* internal_heat_h_tendency

Two interface changes were introduced to complement these fixes.

- The vertical grid struct (GV) was added to geothermal_init

- We have introduced the H_to_MKS scaling factor, which is set to either
  H_to_m or H_to_kg_m2 based on whether the model is in Boussinesq mode.
  This to accommodate diagnostics whose units are based on the
  Boussinesq mode.

The vert_remap_h diagnostic has also been partially rescaled and has
been modified by this patch, but is not yet invariant to H scaling.
The dimensional scaling of three diagnostics has been amended:
* dzRegrid
* vert_remap_h_tendency
* wd

The spatial averaging function global_spatial_mean has also been
modified to run entirely in MKS units, due to the reproducing_sum
function requirement.

A previous change fixed a bug in this function, where only part of the
solution was being scaled.  This new fix restores the original scaling,
but de-scales the final result in order to retain reproducibility.
This patch fixes the dimensional scaling of the following diagnostics:

- massout_flux
- ideal_age
- various DOME derived tracers
- Derived tracer diagnostics:
    - *_adx
    - *_ady
    - *_dfx
    - *_dfx
    - *h_tendency_2d

Many tracer fixes rely on the "flux_" and "conv_" arguments for
rescaling, which is probably not the original intended use, and should
probably be revised in the future.
@codecov-io
Copy link

codecov-io commented Sep 19, 2019

Codecov Report

Merging #1002 into dev/gfdl will increase coverage by 0.05%.
The diff coverage is 92.85%.

Impacted file tree graph

@@             Coverage Diff              @@
##           dev/gfdl    #1002      +/-   ##
============================================
+ Coverage     43.11%   43.16%   +0.05%     
============================================
  Files           213      213              
  Lines         62351    62355       +4     
============================================
+ Hits          26885    26918      +33     
+ Misses        35466    35437      -29
Impacted Files Coverage Δ
src/tracer/MOM_tracer_registry.F90 90.23% <100%> (+0.39%) ⬆️
src/diagnostics/MOM_diagnostics.F90 87.38% <100%> (ø) ⬆️
src/framework/MOM_spatial_means.F90 26.14% <100%> (ø) ⬆️
src/ALE/MOM_ALE.F90 53.22% <100%> (ø) ⬆️
...ameterizations/lateral/MOM_mixed_layer_restrat.F90 86.31% <100%> (+0.19%) ⬆️
src/core/MOM_forcing_type.F90 70.66% <100%> (ø) ⬆️
src/tracer/ideal_age_example.F90 74.12% <100%> (ø) ⬆️
src/framework/MOM_diag_mediator.F90 62.26% <100%> (ø) ⬆️
src/tracer/DOME_tracer.F90 65.78% <100%> (ø) ⬆️
src/framework/MOM_diag_remap.F90 93.08% <100%> (+0.06%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af4a364...a045278. Read the comment docs.

Copy link
Collaborator

@adcroft adcroft left a comment

Choose a reason for hiding this comment

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

@adcroft
Copy link
Collaborator

adcroft commented Sep 19, 2019

Merging but MOM6-examples needs updating...

@adcroft adcroft merged commit 8546837 into mom-ocean:dev/gfdl Sep 19, 2019
@marshallward marshallward deleted the diag_scaling_p2 branch September 19, 2019 22:55
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