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

NCAR to main candidate branch (2023-3-02) #1594

Merged
merged 41 commits into from
Apr 3, 2023

Conversation

alperaltuntas
Copy link
Collaborator

gustavo-marques and others added 30 commits July 26, 2022 09:58
Address comment from reviewer by adding units to covTS and varS.
* Add ``implicit none ; private`` to this module;
* Put module variables into the control structure for this module;
* Add the description of the units for all real variables;
* Add a consistent two-point indent throughout the module .

TODO:
Without further modifications, adding ``private`` to the control
structure of this module will break the model. Currently, MOM.F90
needs access to ``use_stoch_eos``, ``stanley_coeff``, and some of
the diagnostic ids.
…the ideal age

module. This PR also adds the ability to use the actual BL depth that is diagnosed by the
active BL scheme inside the ideal age module (for all ideal age tracers).
Bring in latest main changes (GFDL to main 2022-07-21)
…-main-2022-08-10

Merge GFDL to main (2022-08-10)
In preparation for implementing the option to apply a vertical
smooth filter in the Richardson number multiple times, the
parameter SMOOTH_RI (logical) was renamed to N_SMOOTH_RI (interger).
If N_SMOOTH_RI = 0 (default), smoothing is not performed. If
N_SMOOTH_RI > 0, smoothing will be applied N_SMOOTH_RI times.
Currently, there are two diagnostics related to the gradient Richarson
number and these are described as follows:

* ri_grad_shear         : Gradient Richarson number used by
  MOM_CVMix_shear module;

* ri_grad_shear_smooth  : Smoothed gradient Richarson number used by
  MOM_CVMix_shear module.

The description for ri_grad_shear is misleading. If smoothing is applied,
ri_grad_shear *is not* the RI number used by MOM_CVMix_shear module. In
this commit. I propose to avoid this potential confusion by renaming
ri_grad_shear_smooth to ri_grad_shear_orig and, if N_SMOOTH_RI > 0,
use ri_grad_shear to store the smoothed profiles.

* ri_grad_shear_orig  : Original gradient Richarson number, before smoothing
  was applied. This is part of the MOM_CVMix_shear module and only available
  when N_SMOOTH_RI > 0.

No change in answers for GMOM.
This commit adds the option to smooth the gradient Richardson
number multiple times using a 1-2-1 filter. The number of times
that the filter is applied is controlled by parameter N_SMOOTH_RI.
The correct order is lon_min lon_max lat_min lat_max ...
and not lat_min lat_max lon_min lon_max.
Improvements in the gradient Ri # calculation/diagnosis (MOM_CVMix_shear)
Fix string order in the instructions for defining regional_section
add ML tracer to ideal age module
Testing to see if GH actions is failing due to MPI installation
This patch fixes two issues in the POSIX API.

The `siglongjmp` interface was referencing the wrong symbol (`longjmp`).
While this did not seem to cause any issues, possibly due to some shared
definitions on glibc/BSD platforms, the error was correctly detected by
the Cray compiler.   This patch corrects the C symbol name.

The `sigsetjmp_missing` function, as a default replacement for a missing
`sigsetjmp`, was also defined without a return value, since it always
returns an error if called at runtime.  The Cray compiler raised a
warning about this, so we now assign a return value of -1, although it
is never used.

Thanks to Jim Edwards for reporting these errors.
@alperaltuntas alperaltuntas requested a review from sanAkel March 2, 2023 21:57
@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Merging #1594 (85c1f23) into main (b57ff02) will decrease coverage by 0.04%.
The diff coverage is 16.50%.

❗ Current head 85c1f23 differs from pull request most recent head d1d53bc. Consider uploading reports for the commit d1d53bc to get more accurate results

@@            Coverage Diff             @@
##             main    #1594      +/-   ##
==========================================
- Coverage   37.22%   37.19%   -0.04%     
==========================================
  Files         263      263              
  Lines       73074    73147      +73     
  Branches    13608    13625      +17     
==========================================
+ Hits        27202    27207       +5     
- Misses      40866    40928      +62     
- Partials     5006     5012       +6     
Impacted Files Coverage Δ
src/framework/posix.F90 50.00% <ø> (ø)
src/parameterizations/vertical/MOM_CVMix_shear.F90 11.11% <0.00%> (-0.37%) ⬇️
src/tracer/ideal_age_example.F90 41.96% <17.07%> (-19.64%) ⬇️
src/diagnostics/MOM_obsolete_params.F90 69.66% <100.00%> (+0.34%) ⬆️
src/tracer/MOM_tracer_flow_control.F90 24.64% <100.00%> (ø)

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

Copy link
Collaborator

@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.

I have visually inspected all of the changes in this PR, and apart from a handful of minor inconsistencies with the MOM6 style guide and one possible bug in ideal_age_example.F90 and a missing obsolete_logical() call for "SMOOTH_RI" in MOM_obsolete_params.F90 (see the specific comments for details), these changes all look good to me. Moreover, these changes at least partially resolve several outstanding issues, including #1578 and NCAR#230.

The specific changes I am requesting should be very easy to address. We still need to run them through the GFDL pipeline testing (which we will trigger after the possible bug on lines 402 and 403 of ideal_age_example.F90 is addressed and an obsolete_logical() call is added for SMOOTH_RI) before formal approval from GFDL, but I would be very surprised if this turned up anything.

@jiandewang
Copy link
Collaborator

@alperaltuntas when you do the modification of code, can you help me remove the leading white space at
https://github.com/mom-ocean/MOM6/blob/main/config_src/drivers/nuopc_cap/mom_cap.F90#L1702-L1705
this is a leftover from my previous PR. Thanks in advance.

@alperaltuntas
Copy link
Collaborator Author

@alperaltuntas when you do the modification of code, can you help me remove the leading white space at https://github.com/mom-ocean/MOM6/blob/main/config_src/drivers/nuopc_cap/mom_cap.F90#L1702-L1705 this is a leftover from my previous PR. Thanks in advance.

Done, thanks.

Hallberg-NOAA
Hallberg-NOAA previously approved these changes Mar 17, 2023
@Hallberg-NOAA
Copy link
Collaborator

Thank you @alperaltuntas, all of my comments on the previous version have been nicely addressed. We still need to run our pipeline regression tests (and at the moment our computer is not in a good way, so this might take some doing), but I expect that we will be giving our approval form GFDL after these tests have passed.

@Hallberg-NOAA Hallberg-NOAA dismissed their stale review March 17, 2023 15:29

We need to have our formal pipeline tests pass before we can issue our approval.

@jiandewang
Copy link
Collaborator

we got bitwise identical results in UFS, and John Stephen confirmed the same thing in N. Atlantic regional setting.

Copy link
Collaborator

@jiandewang jiandewang left a comment

Choose a reason for hiding this comment

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

I will be on annual leave for a while starting tomorrow, so here I click the approval before GFDL's pipeline test

Copy link
Collaborator

@kshedstrom kshedstrom left a comment

Choose a reason for hiding this comment

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

I approve these changes.

@marshallward
Copy link
Collaborator

This has passed our regression test, so GFDL can approve it now.

Copy link
Collaborator

@abozec abozec left a comment

Choose a reason for hiding this comment

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

COAPS approves

@marshallward marshallward merged commit 1e54bed into mom-ocean:main Apr 3, 2023
@marshallward
Copy link
Collaborator

Since we are pushing up against some time barriers on our end, I am going to merge this on behalf of @alperaltuntas. Thanks as always to everyone.

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.