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.

Sign up for GitHub

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

Jump to bottom

Dev ncar main candidate 2021 08 31 #1491

Merged
merged 39 commits into from
Sep 14, 2021
Merged

Dev ncar main candidate 2021 08 31 #1491

merged 39 commits into from
Sep 14, 2021

Conversation

gustavo-marques
Copy link
Collaborator

@gustavo-marques gustavo-marques commented Aug 31, 2021 •
edited
Loading

Incorporates the developments done in dev/ncar since Apr 21, 2021. The main contributions are summarized below.

This PR does not change the MOM6-examples answers (ocean.stats.nc) w.r.t. branch main e794c41 when using Cheyenne with the intel/19.0.5 compiler. However, because of a bug fix (misplaced parentheses), this PR will change answers for tracers updated using either tracer_vertdiff_Eulerian or tracer_vertdiff when the optional argument, sfc_flux, is passed.

Features

Bugfixes

Improvements

/glade/scratch/gmarques/bmom.e22.f09_t061_hycom1_N75.mct.surface_fluxes_check.007

External

Review

alperaltuntas and others added 30 commits April 21, 2021 18:03
@alperaltuntas
Merge pull request mom-ocean#186 from gustavo-marques/main-candidate-…
d3f4f0f 
…ncar-2021-04-21

Merging the main candidate from GFDL 2021-03-26
@gustavo-marques
Merge pull request mom-ocean#187 from gustavo-marques/fix_retrieving_…
3812cf6 
…nthreads_nuopc

Add isPresent and isSet when retrieving nthreads
@gustavo-marques
Deallocate Kd_bkgnd and Kv_bkgnd
14e2334 
These arrays were not being deallocated which was causing a
memory leak problem.
@alperaltuntas
Merge pull request mom-ocean#188 from gustavo-marques/fix_mem_leak_bkgnd
d1de7c8 
De-allocate Kd_bkgnd and Kv_bkgnd
@alperaltuntas
introduce a new wave coupling method: VR12-MA (not fully implemented…
0d59156 
… yet)
@gustavo-marques
Fixed a misplaced parentheses
276954f 
This commit fixes misplaced parentheses in the tridiagonal solvers used in
subroutines tracer_vertdiff_Eulerian and tracer_vertdiff. This bug has been
reported in https://github.com/NOAA-GFDL/MOM6/issues/1415

This commit also changes the mask condition used in these subroutines.
@gustavo-marques
CFCs implementation via NUOPC cap
3aade32 
This commit adds a new module (MOM_CFC_cap) that can used to
simulate chlorofluorocarbons (CFCs) tracers. It also includes
the modifications needed to 1) allocate arrays in the fluxes and
surface types, and 2) activate the MOM_CFC_cap module via NUOPC cap.
@alperaltuntas
introduce the lamult import from ww3 in nuopc cap and add a diag
f45659d
@alperaltuntas
Merge pull request mom-ocean#189 from gustavo-marques/ncar_cfcs_imple…
b7e0fda 
…mentation

* Implements a module to simulate CFCs via NUOPC cap
@alperaltuntas
import ifrac and u10_sqr only when CFC module is active
b767a13
@gustavo-marques
Merge pull request mom-ocean#190 from NCAR/fix_cfc_import
230a8b8 
Fix NUOPC import of fields required for the new CFC module
@alperaltuntas
merge latest dev/ncar into ww3_coupling_vr12ma
bc65e4f
@alperaltuntas
Enable the export of ice fraction when wave coupling is on.
5f756ab 
The export of ice fraction was previously added for the
newly added CFC module.
@alperaltuntas
set lamult to 1 below ice
7d85ab2
@alperaltuntas
move lamult from forces to fluxes
997636e
@gustavo-marques
Changes the order of calculation
7622911 
In a previous commit, the expression

tr(i,j,1) = (b1(i)*h_tr)*tr(i,j,1) + sfc_src(i,j)

was change to

tr(i,j,1) = b1(i)*(h_tr*tr(i,j,1) + sfc_src(i,j))

in 4 lines. This changed the order in which b1(i)*h_tr*tr(i,j,1) is evaluated,
even if sfc_src(i,j) is 0.0. Because the CESM configurations use the
passive tracer tridiagonal solver for T and S, the modification above changed
answers in these configurations. To recover previous answers the expression must
be

tr(i,j,1) = (b1(i)*h_tr)*tr(i,j,1) + b1(i)*sfc_src(i,j)
@alperaltuntas
merge dev/ncar with main 210723
3117b98
@gustavo-marques
Merge pull request mom-ocean#193 from NCAR/merge_main_210802
69f653a 
Merge main from GFDL (210723)
@alperaltuntas
Merge pull request mom-ocean#191 from gustavo-marques/recover_answer_…
3e1b0cd 
…change_in_CFC_PR

* Recover answer change from CFC PR
@alperaltuntas
bringing in vr12ma ww3 coupling changes to the latest dev/ncar and re…
7ebf3b0 
…solving conflicts
@alperaltuntas
Minor fixes for the WW3 coupling option VR12-MA
1412771
@alperaltuntas
Introduce changes needed for CVMix KPP module for the VR12-MA wave co…
2d385a0 
…upling option
@alperaltuntas
fix doxygen error: add comments for wave method strings
9fe68ac
@alperaltuntas
fix omp error in KPP module: add lamult to shared clauses
80e1e2e
@alperaltuntas
change VR12-MA wave_method string to EFACTOR
734e537
@alperaltuntas
Refactor the way Langmuir entrainment and enhancement factor are comp…
d734536 
…uted and applied: (1) When available, pass the enhancement factor received from WW3. Otherwise, let CVmix compute the enhancement factor. (2) Instead of explicitly multiplying diffusivities and viscosities with the enhancement factor, let CVMix handle enhancement internally.
@klindsay28
Various dimension rescaling fixes
9011801 
correct scale argument in get_param and chksum calls

add dimension scaling terms to some expressions

add some chksum calls
@klindsay28
dimension rescaling fixes for MOM_CFC_cap tracers
e87adb8
@klindsay28
let register_tracer handle flux_units for MOM_CFC_cap tracers
bc19819
@klindsay28
correct prepending inputdir to CFC_BC_file
fa96ae3
@jiandewang
Copy link
Collaborator

jiandewang commented Aug 31, 2021 via email

@kshedstrom
Copy link
Collaborator

I approve this PR.

@marshallward
Copy link
Collaborator

The tc1 tests - and possibly others - are detecting regressions in the diagnostics, e.g.

< h-point: mean=  -5.7496361301369858E-01 min=  -1.0000000000000000E+00 max=   1.1415525114155251E-04 ocean_model-age
make: *** [Makefile:473: tc1.a.regression.diag] Error 1
< h-point: c=     10412 ocean_model-age
---
> h-point: mean=  -5.7496361301369858E-01 min=  -1.0000000000000002E+00 max=   1.1415525114155251E-04 ocean_model-age
> h-point: c=     15150 ocean_model-age
2945,2946c2945,2946
< h-point: mean=  -1.5971211472602740E-04 min=  -2.7777777777777778E-04 max=   3.1709791983764586E-08 ocean_model-age_tendency
< h-point: c=     19164 ocean_model-age_tendency
---
> h-point: mean=  -1.5971211472602740E-04 min=  -2.7777777777777783E-04 max=   3.1709791983764586E-08 ocean_model-age_tendency
> h-point: c=     18888 ocean_model-age_tendency
2954c2954
< h-point: c=     18833 ocean_model-ageh_tendency

(Not a complete list, just the header of the diff)

It looks to be a last-bit difference in the min value, though the checksum different is also evident.

If this is intentional then we can override it, but if not then it should be investigated.

@gustavo-marques
Copy link
Collaborator Author

The tc1 tests - and possibly others - are detecting regressions in the diagnostics, e.g.

< h-point: mean=  -5.7496361301369858E-01 min=  -1.0000000000000000E+00 max=   1.1415525114155251E-04 ocean_model-age
make: *** [Makefile:473: tc1.a.regression.diag] Error 1
< h-point: c=     10412 ocean_model-age
---
> h-point: mean=  -5.7496361301369858E-01 min=  -1.0000000000000002E+00 max=   1.1415525114155251E-04 ocean_model-age
> h-point: c=     15150 ocean_model-age
2945,2946c2945,2946
< h-point: mean=  -1.5971211472602740E-04 min=  -2.7777777777777778E-04 max=   3.1709791983764586E-08 ocean_model-age_tendency
< h-point: c=     19164 ocean_model-age_tendency
---
> h-point: mean=  -1.5971211472602740E-04 min=  -2.7777777777777783E-04 max=   3.1709791983764586E-08 ocean_model-age_tendency
> h-point: c=     18888 ocean_model-age_tendency
2954c2954
< h-point: c=     18833 ocean_model-ageh_tendency

(Not a complete list, just the header of the diff)

It looks to be a last-bit difference in the min value, though the checksum different is also evident.

If this is intentional then we can override it, but if not then it should be investigated.

The PR description has been updated. I now mention that this PR will change answers for tracers that use either tracer_vertdiff_Eulerian or tracer_vertdiff, which is the case for ideal age in the tc1 regression test.

@alperaltuntas alperaltuntas mentioned this pull request Sep 2, 2021
Merged
sanAkel
sanAkel approved these changes Sep 2, 2021
View reviewed changes
Copy link
Collaborator

@sanAkel sanAkel left a comment

Choose a reason for hiding this comment

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

👍 Thanks for those comments.

@jiandewang
Copy link
Collaborator

@gustavo-marques: I just returned back from vacation, will test it and get back to you in 1-2 days.

@jiandewang
Copy link
Collaborator

works fine in UFS.

@abozec
Copy link
Collaborator

abozec commented Sep 10, 2021

I approve this PR also

@Hallberg-NOAA
Copy link
Collaborator

I have run the MOM6-examples pipeline tests manually with this PR, and the answers are all unchanged.

I approve this PR, although there are two issues that it raises that I think warrant further discussion:

  1. We now have 3 distinct implementations of CFC being used by MOM6 (via the generic tracers, the new MOM_CFC_cap.F90 and MOM_OCMIP2_CFC.F90) , I think that a careful comparison and reconciliation of these various codes could be an instructive exercise for setting a template for other tracer packages.
  2. Tests related to the new fluxes%lamult array have resulted in duplicated KPP calls in MOM_diabatic_driver.F90. Perhaps we could explore whether a slightly different code construct could give the same algorithmic flexibility with less duplication.

@marshallward
Copy link
Collaborator

I verified that this passed our regression test after fixing some conflicts (due to changes after the PR was submitted).

There is a new (expected) parameter as well:

  • USE_CFC_CAP

So I will go ahead and merge this to main.

@marshallward marshallward merged commit c09e199 into mom-ocean:main Sep 14, 2021
This was referenced Sep 15, 2021
Closed
Closed
@Hallberg-NOAA Hallberg-NOAA mentioned this pull request Sep 16, 2021
Closed
@gustavo-marques gustavo-marques deleted the dev-ncar-main-candidate-2021-08-30 branch February 17, 2022 15:39
@klindsay28 klindsay28 mentioned this pull request May 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alperaltuntas alperaltuntas alperaltuntas left review comments

@Hallberg-NOAA Hallberg-NOAA Hallberg-NOAA left review comments

@sanAkel sanAkel sanAkel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@gustavo-marques @jiandewang @kshedstrom @marshallward @abozec @Hallberg-NOAA @alperaltuntas @sanAkel @klindsay28