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 master candidate 2018-10-15 #862

Merged
merged 219 commits into from
Nov 6, 2018
Merged

Dev master candidate 2018-10-15 #862

merged 219 commits into from
Nov 6, 2018

Conversation

adcroft
Copy link
Collaborator

@adcroft adcroft commented Oct 20, 2018

This PR is to merge dev/gfdl as of 2018-10-15 onto dev/master. There are a dozen or so merges here but the big one is #859 that touches 108 files. There are no (known) answer changes but parameter documentation files are affected. There is a corresponding branch of MOM6-examples with updated doc files but the parameter docs for you own experiment repositories should be checked.

As per usual feedback from @gustavo-marques, @awallcraft, @kshedstrom and @jiandewang is asked for before we accept this PR.

PS> Sorry it took a week. I tried creating this PR three times this week but was interrupted each time. Closing my door seems to have become ineffective.

kshedstrom and others added 30 commits July 10, 2018 14:01
@kshedstrom
Merge remote-tracking branch 'gfdl/dev/gfdl' into oblique_fuss
40c2043
@kshedstrom
Merge remote-tracking branch 'gfdl/dev/gfdl' into oblique_fuss
106b3e0
@Hallberg-NOAA
Corrected two openMP directives
50b850d 
  Corrected openMP directives in two places, so MOM6 now compiles with openMP
enabled. The variable local_strain had recently been added to
horizontal_viscosity, but it was omitted from an openMP directive.  A recent
change had left an incomplete openMP directive around KPP_get_BLD in diabatic.
All solutions (at least without openMP on) are bitwise identical.
@kshedstrom
Time-filter on oblique OBCs.
ad3961c 
- also set default OBC vorticity, strain to freeslip.
@kshedstrom
Fix nudging with oblique OBCs.
12410a1
@Hallberg-NOAA
+Added stress_mag to ice_ocean_boundary_type
e0ca46b 
  Added a new element, stress_mag, with the time-mean of the magnitude of the
wind stresses at tracer points, to the ice_ocean_boundary_type.  It is not yet
being used, so all answers are bitwise identical.
@adcroft
Merge pull request #821 from Hallberg-NOAA/correct_openMP
b2b8bf7 
Corrected two openMP directives
@adcroft
Merge branch 'dev/esmg' of https://github.com/ESMG/MOM6 into ESMG-dev…
d3b63db 
…/esmg
@adcroft
Merge branch 'ESMG-dev/esmg' into dev/gfdl
f738996
@adcroft
Merge pull request #825 from Hallberg-NOAA/add_stress_mag
3248505 
+Added stress_mag to ice_ocean_boundary_type
@adcroft
Merge branch 'dev/master' into dev/gfdl
7030e56
@kshedstrom
Adding OBLIQUE_TAN and OBLIQUE_GRAD options.
8d794ef
@kshedstrom
Merge remote-tracking branch 'gfdl/dev/gfdl' into dev/esmg
53cd176
@Hallberg-NOAA
+Use stress_mag to set ustar
c95d513 
  Added code to use IOB%stress_mag to set ustar if is allocated.  The code to
set stress_mag in SIS2 is equivalent to that in MOM6, so the answers are
currently unchanged if this new option is used. Also rearranged the code setting
the wind stresses, ustar, and other forcing fields so they are more logically
grouped.  All answers are bitwise identical in test cases, but there are new
options to allow the sea-ice or coupler to set ustar differently.
@Hallberg-NOAA
+Added extract_IOB_stresses
e1762f5 
  Added a new subroutine, extract_IOB_stresses, to obtain the wind stresses and
friction velocities from the ice-ocean-boundary type into simple arrays that are
provided as optional arguments.  All answers are bitwise identical.
@Hallberg-NOAA
Always set G%Domain_aux
4b859d4 
  G%Domain_aux points to a non-symmetric MOM6 domain.  It had previously only
been set if G%Domain is symmetric, but was otherwise not associated.  Now if
G%domain is itself non-symmetric, G%domain_aux simply points back to G%domain.
G%domain_aux can now be used more widely without causing problems.  All answers
are bitwise identical.
@Hallberg-NOAA
Set stresses via extract_IOB_stresses
7507e2d 
  Replaced the code setting the wind stresses in code_IOB_to_forces with a call
to extract_IOB_stresses.  Also streamlined extract_IOB_stresses to avoid extra
unnecessary communications.  All answers are bitwise identical.
@Hallberg-NOAA
Code cleanup in MOM_surface_forcing.F90
0d18946 
  Code cleanup in MOM_surface_forcing.F90 to reduce memory use.  All answers are
bitwise identical.
@Hallberg-NOAA
Set ustar in fluxes via extract_IOB_stresses
b15f514 
  Set ustar in fluxes via extract_IOB_stresses, using sub-optimal expressions
involving division by mean density rather than multiplication by its reciprocal
to reproduce what had been done in set_derived_forcing_fields.  All answers are
bitwise identical.
@Hallberg-NOAA
+Changed interface to forcing_accumulate
f20c3f1 
  Turned forces into an optional argument to forcing_accumulate and changed the
order of the list of arguments.  Forces is no longer needed when the pressure
and ustar fields are properly set in the temporary fluxes array.  The forces
argument is now omitted from the call to forcing_accumulate in
update_ocean_model, and the call to set_derived_forcing_fields has been
eliminated.  All answers are bitwise identical.
@Hallberg-NOAA
(+)Restored the interface to forcing_accumulate
c02b134 
  Restored the interface to forcing_accumulate to what it had been previously,
and added a new subroutine, fluxes_accumulate, that uses the newer interface,
with the new forcing_accumulate calling fluxes_accumulate.  This new interface
is now in use in update_ocean_model.  In addition, set_net_mass_forcing now
calls get_net_mass_forcing to eliminate duplicated code.  All answers are
bitwise identical, and slightly older public interfaces have been restored to
avoid code conflicts with MOM6 drivers outside of coupled_driver.
@Hallberg-NOAA
+Added optional arguments to convert_IOB_to_forces
13f9303 
  Added two new optional arguments to convert_IOB_to_forces to allow it to do
a running time average of ustar, matching what had previously been done only
for ustar in the fluxes type.  Also added the new element dt_force_accum to the
mech_forcing type to enable this averaging.  All answers are bitwise identical,
although there are new optional arguments to a publicly visible routine.
@Hallberg-NOAA
Consolidate dynamic & thermodynamic forcing setup
5f253d4 
  Consolidated the code in update_ocean_model that set up the dynamic and
thermodynamic forcing structures.  This takes advantage of the recently added
optional arguments to convert_IOB_to_forces to do time averaging of ustar.
All answers are bitwise identical.
@Hallberg-NOAA
(*)Allow for fractional second coupling timesteps
c54a25a 
  By replacing several set_time calls that quantize times at whole numbers of
seconds with calls to real_to_time_type, the MOM6 coupled timesteps can now
be integer numbers of ticks (fractional seconds).  This could change answers if
MOM6 were called with non-integer second timesteps, but in all existing test
cases this is not the case, so the answers are bitwise identical.
@Hallberg-NOAA
Use real_to_time_type in 63+year segment clock cor
d9e9457 
  Use real_to_time_type in long-time (>63 year segment) ocean-only model clock
correction for improved accuracy with fractional timesteps and very long run
segments.  All answers are bitwise identical in existing test cases.
@Hallberg-NOAA
Corrected description of SINGLE_STEPPING_CALL
974662e 
  Corrected the documentation in the get_param call for SINGLE_STEPPING_CALL,
that was inadvertently messed up two commits ago.  All answers are bitwise
identical, and inadvertent changes to the MOM_parameter_doc files have been
reversed.
@wrongkindofdoctor
Merge pull request #1 from NOAA-GFDL/dev/gfdl
171e0c8 
Pull latest MOM6 changes into dev/gfdl fork
@Hallberg-NOAA
+Added APPROX_NET_MASS_SRC & moved RESTORE_SALINITY
a30575d 
  Added code to store an estimate of the net mass source in the mech_forcing
type, along with the new run time parameter APPROX_NET_MASS_SRC that controls
this behavior.  This estimate should be correct for coupled models, but may be
off with data overrides or restoring.  Because forces%net_mass_src is not yet
used in the solution, that answers are invariant to the use of this option.

  Also moved the get_param calls for RESTORE_SALINITY and RESTORE_TEMPERATURE
into surface_forcing_init, and eliminated the corresponding arguments from
surface_forcing_init and convert_IOB_to_fluxes, because these parameters were
not used in the top-level MOM6 code. Also added a new flag, net_mass_src_set, to
the mech_forcing type and dOxygenized the comments in and surrounding the
surface_forcing_CS.  By default, all answers are bitwise identical, but there is
a new run-time parameter, changes to publicly visible interfaces, and the
MOM_parameter_doc files change.
@Hallberg-NOAA
+Removed forcing type arg from mech_forcing_diags
6c46811 
  Moved 4 diagnostics from mech_forcing_diags to forcing_diagnostics and removed
the now unused (thermodynamic) forcing type argument fluxes from
mech_forcing_diags, so that the location of the diagnostics better reflects
their use in stepping MOM6.  All calls to mech_forcing_diags in the drivers were
changed accordingly.  Also, a new element, nstep_thermo, was added to the
ocean_state_type to allow dynamic and thermodynamic calls to update_ocean_model
to be counted separately, and some additional calls now only occur if the
dynamics or thermodynamics are being stepped.  All answers are bitwise
identical, but one publicly visible interface has changed.
removed underscore from allocated statement in MOM_generic_tracer_col…
0cb2d87 
…umn_physics routine
Hallberg-NOAA and others added 8 commits October 11, 2018 08:44
@Hallberg-NOAA
+Recast calc_isoneutral_slope to work in units of Z
8738e29 
  Recast the calc_isoneutral_slope take interface heights (the argument e) in
units of Z, and internally to use vertical height units of Z in place of m for
dimensional consistency testing.  Also recast calc_slope_functions_using_just_e
to take interface heights in units of e.  All answers are bitwise identical in
the MOM6 test cases, including rescaling Z over a large range.
@Hallberg-NOAA
+Recast vert_fill_TS to work with units of Z
1402a86 
  Recast the vert_fill_TS take the diffusivity argument in units of Z2 s-1, and
for the private version in calc_isoneutral_slopes eliminated the timescale
argument (which had previously been hard-coded to 1.0).  Rescaling the
diffusivities vert_fill_TS required added in a new GV argument to VarMix_init.
All answers are bitwise identical in the MOM6 test cases, including rescaling Z
over a large range.
@Hallberg-NOAA
Fixed rescaling of eta_av unsplit stepping code
4bf6386 
  Corrected the dimensional rescaling of eta_av in step_MOM_dyn_unsplit and
step_MOM_dyn_unsplit_RK2 to go to H.  This only occurs with Boussinesq code, and
the answers do not change if H_to_m is 1, as is often the case, and only
diagnostics are impacted in ocean only cases.  This code appears not to be
adequately tested with the MOM6_examples test suite, which was bitwise identical
with this change.
@adcroft
Merge branch 'dev/master' into dev/gfdl
587c9b7 
Conflicts:
- config_src/mct_driver/ocn_comp_mct.F90 (I treated dev/master as the authoritative version since that has the latest code from NCAR)
@Hallberg-NOAA
Recast MOM_wave_speed to work in units of Z
a2ab18d 
  Recast the internal calculations in MOM_wave_speed to use vertical height
units of Z in place of m for dimensional consistency testing.  Several probable
bugs were highlighted in comments but not corrected. All answers are bitwise
identical in the MOM6 test cases, including rescaling Z over a large range.
@Hallberg-NOAA
Recast MOM_structure to work in units of Z
b806a34 
  Recast the internal calculations in MOM_structure to use vertical height units
of Z in place of m for dimensional consistency testing.  At one point the code
goes into a complicated iterative tridiagonal solver, and this part of the
algorithm reverts to working in m (for now). All answers are bitwise identical
in the MOM6 test cases, including rescaling Z over a large range.
@Hallberg-NOAA
Merge branch 'dev/gfdl' into test_depth_units
0569c2b
@adcroft
Merge branch 'Hallberg-NOAA-test_depth_units' into dev/gfdl
ef9ecf2
@gustavo-marques
Copy link
Collaborator

I am getting SIGSEGV, see below. I've updated other submodules (e.g., SIS2, icebergs) and did a clean built using dev-master-candidate-2018-10-15.

gmarques@r12i6n34:/glade/work/gmarques/Cheyenne-stats-MOM6-examples> make -f Gitlab/Makefile.run intel_all MEMORY=dynamic_symmetric LOG=
echo build/intel/repro/dynamic_symmetric/coupled_AM2_LM3_SIS/MOM6"("90")" "=>" MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel
build/intel/repro/dynamic_symmetric/coupled_AM2_LM3_SIS/MOM6(90) => MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel
cd MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L && rm -rf Depth_list.nc CPU_stats.intel time_stamp.out ocean.stats.intel RESTART FAIL && mkdir RESTART
cd MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L && tic=$(date +%s) && (OMP_NUM_THREADS=1 KMP_STACKSIZE=512m NC_BLKSZ=1M time mpiexec_mpt -n 90 ../../../build/intel/repro/dynamic_symmetric/coupled_AM2_LM3_SIS/MOM6 > log.intel.out || touch FAIL;) 2>&1 | egrep -v "ing coupler_init| initializ|ing " | sed 's,^,MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: ,' ; toc=$(date +%s) ; echo $(($toc-$tic)) > walltime.intel.out
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: forrtl: severe (174): SIGSEGV, segmentation fault occurred
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: Image              PC                Routine            Line        Source             
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: MOM6               0000000002CD3241  Unknown               Unknown  Unknown
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: MOM6               0000000002CD137B  Unknown               Unknown  Unknown
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: MOM6               0000000002C5C894  Unknown               Unknown  Unknown
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: MOM6               0000000002C5C6A6  Unknown               Unknown  Unknown
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: MOM6               0000000002BDF489  Unknown               Unknown  Unknown
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: MOM6               0000000002BE3B96  Unknown               Unknown  Unknown
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: libpthread-2.19.s  00002AAAAB603870  Unknown               Unknown  Unknown
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: MOM6               000000000279D895  coupler_types_mod        3776  coupler_types.F90
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: MOM6               00000000019CC1B8  mom_surface_forci        1518  MOM_surface_forcing.F90
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: MOM6               000000000045D006  coupler_main_IP_c        1922  coupler_main.F90
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: MOM6               0000000000453192  MAIN__                    560  coupler_main.F90
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: MOM6               000000000040531E  Unknown               Unknown  Unknown
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: libc-2.19.so       00002AAAAB832B25  __libc_start_main     Unknown  Unknown
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: MOM6               0000000000405229  Unknown               Unknown  Unknown
MOM6-examples/coupled_AM2_LM3_SIS/CM2G63L/ocean.stats.intel: forrtl: severe (174): SIGSEGV, segmentation fault occurred

Currently Loaded Modules:

  1. python/3.6.4 2) ncview/2.1.7 3) ncarenv/1.2 4) intel/17.0.1 5) netcdf/4.6.1 6) ncarcompilers/0.4.1 7) mpt/2.15f

@jiandewang
Copy link
Collaborator

jiandewang commented Oct 23, 2018 via email

@jiandewang
Copy link
Collaborator

jiandewang commented Oct 23, 2018 via email

@adcroft
Copy link
Collaborator Author

adcroft commented Oct 23, 2018

Thanks, @jiandewang . I'm patching 2+3 as you suggested and have a different fix for 1. Testing now and will update PR shortly...

  1. I think this is because ISS%hmask is passed to a function that has the argument inout but in fact hmask is not changed so that lower function was incorrect.
  2. Yes, annoyingly wherever there is a halo update G needs to be inout.
  3. Similarly, there's a halo update in the smoothing function so G has to be inout all the way through KPP too.

@adcroft
Corrected intent of some variables
204a767 
- ice_shelf_advect_temp_*() has G intent(inout) but should be intent(in)
- KPP_compute_BLD() and KPP_smooth_BLD() had G intent(in), now intent(inout)
- set_visc_init() had G intent(in), now intent(inout)
- Thanks to @jiandewang for reporting issue when evaluating PR #862
@Hallberg-NOAA
Copy link
Collaborator

This PR has problems (will not compile and does not thread properly) with the openMP directives; these issues are addressed for dev/GFDL by https://github.com/NOAA-GFDL/MOM6/pull/866.

@Hallberg-NOAA @adcroft
Corrected openMP directive errors in recent commit
6c98b8e 
  Corrected a number of openMP directives that had been broken by a recent
set of commits.  The code would not compile with openMP enabled without these
changes.  All answers are bitwise identical, and answers have been verified to
reproduce with 1 and 2 threads for the SIS2_cgrid_bergs test case.
@codecov-io
Copy link

codecov-io commented Oct 25, 2018 •
edited
Loading

Codecov Report

Merging #862 into dev/master will decrease coverage by 0.32%.
The diff coverage is 49.15%.

Impacted file tree graph

@@              Coverage Diff              @@
##           dev/master    #862      +/-   ##
=============================================
- Coverage       53.62%   53.3%   -0.33%     
=============================================
  Files             215     216       +1     
  Lines           56385   56985     +600     
=============================================
+ Hits            30237   30374     +137     
- Misses          26148   26611     +463
Impacted Files Coverage Δ
config_src/solo_driver/MESO_surface_forcing.F90 0% <ø> (ø) ⬆️
...c/parameterizations/vertical/MOM_set_viscosity.F90 70.54% <ø> (-1.16%) ⬇️
...parameterizations/vertical/MOM_full_convection.F90 0% <ø> (ø) ⬆️
src/initialization/MOM_fixed_initialization.F90 64.78% <ø> (ø) ⬆️
src/user/shelfwave_initialization.F90 0% <ø> (ø) ⬆️
src/user/user_revise_forcing.F90 100% <ø> (ø) ⬆️
src/tracer/dyed_obc_tracer.F90 0% <ø> (ø) ⬆️
src/tracer/DOME_tracer.F90 77.88% <ø> (ø) ⬆️
...rameterizations/vertical/MOM_entrain_diffusive.F90 91.9% <ø> (-0.13%) ⬇️
src/tracer/MOM_tracer_Z_init.F90 0% <ø> (ø) ⬆️
... and 171 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 c77ad42...a3b8f51. Read the comment docs.

@adcroft
Copy link
Collaborator Author

adcroft commented Oct 25, 2018

I've updated this PR with two commits to address issues found by @jiandewang and @Hallberg-NOAA . Back to you all... @jiandewang, @awallcraft, @kshedstrom, @gustavo-marques

@gustavo-marques do we need to talk about getting this to work for you?

@gustavo-marques
Copy link
Collaborator

Thanks @adcroft, after checking MOM6-example recursively and doing a clean build the tests passed. I will re-run the tests now and @alperaltuntas will run the CESM tests.

@alperaltuntas
Copy link
Collaborator

CESM tests are failing due to a change in the arguments of mech_forcing_diags subroutine (commit 6c46811). I will fix the issue, re-run the CESM tests and will send a PR to this candidate if tests pass.

@jiandewang
Copy link
Collaborator

jiandewang commented Oct 26, 2018 via email

@alperaltuntas
Copy link
Collaborator

@adcroft et al., PR #871 solves our problem in MCT cap.

@gustavo-marques, The reason we get answer changes is because this candidate does not include the following commit: NCAR@6f12f84 , which is fine, but something we should be wary of when merging back to dev/ncar.

Hallberg-NOAA and others added 2 commits October 26, 2018 09:18
@Hallberg-NOAA
Merge pull request #871 from alperaltuntas/fix_mct_candidate-2018-10-15
a114303 
fix mech_forcing_diags args
@adcroft
Fix intent for geothermal_init()
a3b8f51 
- @jiandewang reported that the argument G should be intent(inout).
@adcroft
Copy link
Collaborator Author

adcroft commented Oct 26, 2018

Two more patches have been made due to @alperaltuntas and @jiandewang . Hopefully no more... ?

@gustavo-marques
Copy link
Collaborator

Now this PR passes all our test cases.

@jiandewang
Copy link
Collaborator

jiandewang commented Oct 26, 2018 via email

@kshedstrom
Copy link
Collaborator

kshedstrom commented Nov 5, 2018 via email

@adcroft adcroft merged commit a3b8f51 into dev/master Nov 6, 2018
@adcroft adcroft deleted the dev-master-candidate-2018-10-15 branch November 7, 2018 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.