Redefine ~500 pointers as local or stack variables #5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note that I used ~30 commits here in order to diagnose potential future problems, so this should probably not be squash-merged. |
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #5 +/- ##
==========================================
Coverage 29.15% 29.16%
==========================================
Files 239 239
Lines 71463 71108 -355
==========================================
- Hits 20833 20736 -97
+ Misses 50630 50372 -258
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I have examined the entirety of this PR, and I am generally strongly in support of it. However, I have three questions before approving this.
-
In the cases where there are multiple large trees of control structures, would it make sense to make the top-level control structures allocatable, rather than having them always exist even if they are not used. This issue is most prominent in MOM.F90, MOM_diabatic_driver.F90, MOM_set_diffusivity.F90 and MOM_PressureForce.F90, but it could pertain elsewhere.
-
Whether a pointer to a control structure was used before to indicate whether modules are initialized, but the tests for initialization have been removed. Would it be advisable to restore these tests, but using a new initialized flag in the various control structures?
-
In 3 cases the
! local variablescomments separating subroutine arguments from local variables were removed, from line 5049 of MOM_open_boundary.F90, line 235 of DOME_2d_initialization.F90, and line 46 of BFB_initialization.F90. Was this advertent, and if so what was the reasoning behind removing these comments, which had been encouraging?
|
All good questions, and I don't have certain answers for any of them. But I'll comment below.
We can also talk more about it. |
* Change `hor_visc_CS` pointers to locals
* equilibrium_value removed from CS and is now local
* MEKE_CS function arguments to stack
* `mixedlayer_restrat_CS` pointers changed to locals MLD_filtered[_slow] pointed moved to allocatables * MLD argument for `mixedlayer_restrat` Restore MLD pointer Passing an uninitialized array is problematic, though passing a pointer to an uninitialized array is not. This can be addressed when vertvisc_type is resolved.
* `thickness_diffusion_CSp` is moved to local where possible * All arrays and most other pointer content is moved to either allocatable or local to the type.
* `tidal_forcing_CS` pointers are removed, and its fields are converted
to allocatables.
- Note that references are retained in the pressure force and
barotropic CS instances, to avoid copies.
Still working through that one...
* Internal tide CS pointer removal (int_tide_CS) * Diabatic driver's `int_tide_CSp` renamed to `int_tide` I am unsure if the instance of int_tide_CS in the diabatic driver (where it is created) needs to be declared as target. Seems not, but watch this issue.
This removes just about all of the entrain_diffusive pointers (except the diag_ctrl). There is a very minor incongruity with `just_read_params`, which was originally used to deallocate the CS, which might alter some removed `if(associated(CS))` checks. But it seems this is not really a problem, since the calls to entrainment_diffusion() are inside regions unreachable when this flag (from `CS%useALEalgorithm`) is true.
* `tidal_mixing_CS` pointers moved to locals * `tidal_mixing_CSp` in diffusivity renamed to `tidal_mixing` * Most of the pointer-declared fields converted to allocatables. * Local `dd` pointers to `CS%dd` removed * Reorder calculate_tidal_mixing (and sub-procedure) args
* `geothermal_CS` converted from pointers to locals * Instance of `geothermal_CS` in diabatic driver changed to local
* Diabatic driver `opacity_CSp` renamed to `opacity`, changed to local * Instances of `optics` and `opacity_CS` converted to locals * Fields in `opacity_CS` and `optics_type` changed to allocatables
* `bulkmixedlayer_CS` pointers moved to locals * Diabatic driver renamed variables: * `bulkmixedlayer` flag -> `use_bulkmixedlayer` * `bulkmixedlayer_CSp` -> `bulkmixedlayer` * Some redundant documentation removed
* `energetic_PBL_CS` pointers changed to locals in main module * `energetic_PBL_CSp` changed to local and renamed to `energetic_PBL` in diabatic driver.
* `regularize_layer_CS` pointers in module moved to local * `regularize_layer_CSp` in diabatic driver moved to local * diabatic drriver `regularize_layer_CSp` renamed to drop `_CSp`
* `CVMix_conv_CS` in diabatic driver renamed to `CVMix_conv` * Pointer instances of `CVMix_conv_CS` changed to locals * `CVMix_end` function removed, since it did nothing.
* `MEKE_type` pointers of many arguments and control structures changed
from pointers to locals
* `MEKE` input removed from the following:
* `initialize_dyn_unsplit`
* `initialize_dyn_unsplit_RK2`
* Many `associated(MEKE)` checks have been removed, and now rely on
associations of individual components within MEKE
This is just "passing the buck" and not solving the underlying issue
of decision-by-allocation, but it's closer to a true solution.
* Pointer fields inside `MEKE_type` converted to allocatables
`associated()` tests for them replaced with `allocatable()`
* `VarMix_CS` pointer instances redefined as locals * `associated(VarMix)` tests replaced with `VarMix%use_variable_mixing`. This ought to be identical, since `VarMix_init()` deallocates the CS if this flag is unset (False), and the function is always called. * VarMix arrays changed from pointers to allocatables
* `wave_speed_CS` pointers redefined as locals * `wave_speed_CSp` in varmix and diagnostics renamed to `wave_speed` * `S` and `T` pointers removed wave speed update
* Most `MOM_restart_CS` instances changed from pointers to locals * `MOM_restart_CS` removed from unsplit dyncore subroutines * `restart_CSp` renamed to `restart_CS` in many functions
* Instances of `barotropic_CS` pointers changed to locals * `barotropic_CSp` removed from MOM module, as it was unused * `frhat[uv]1` converted to allocatable * `BT_OBC` field pointers to allocatable
* `wave_structure_CS` instances changed from pointer to local * T and S aliases to `tv` in `MOM_wave_structure` removed * `wave_structure_CSp` renamed to `wave_struct` in internal tides
* Some instances of `ALE_sponge_CS` declared as local * Redfined many allocatable arrays declared as pointers Much of this module was left unmodified, due to a lot of decision making based on the associated status of the CS.
* diagnostic field pointers changed to allocatables * `safe_alloc_ptr` calls replaced with `allocate()` * Some instances of `diagnostics_CS` passed as type * `diagnostics_CS` in MOM_mod moved to stack
Convert `set_visc_CS` pointer instances to locals
* `EOS_type` pointers in MOM_EOS_mod moved to locals * Various `EOS_type` pointers changed to local * `eqn_of_state` removed from the following: - `adjustment_initialize_temperature_salinity` - `BFB_set_coord` - `dense_water_initialize_TS` - `DOME2d_initialize_temperature_salinity` - `dumbbell_initialize_temperature_salinity` - `Neverworld_initialize_thickness` - `Rossby_front_initialize_temperature_salinity` - `seamount_initialize_temperature_salinity` - `sloshing_initialize_temperature_salinity` - `USER_initialize_temperature_salinity` - `USER_set_coord` * `EOS` removed from neutral diffusion unit test NOTE: eqn_of_state in MOM_mod is retained, since there are many checks of pointer associated of `tv%eqn_of_state`.
Redefine Coriolis advection pointers as locals
* The following control structures were moved to locals: * `PressureForce_CS` * `PressureForce_FV_CS` * `PressureForce_Mont_CS` * The `*_end()` functions no longer do anything and were removed
These pointers were defined as locals * `MOM_continuity_CS` * `MOM_continuity_PPM_CS`
This patch replaces many pointers in the OBC with local or allocatable variables. Notes: * `OBC` redeclared as `target` in some places for `segment` pointers * `field_names` removed from `OBC_segment_type` * `Tr_Reg` removed from `set_tracer_data` * `OBC` removed from `deallocate_OBC_segment_data`
marshallward
force-pushed
the
no_more_pointers
branch
from
fa40b5a
to
d00c90b
Compare
Hallberg-NOAA
approved these changes
Nov 22, 2021
There was a problem hiding this comment.
I approve this PR. There is a related PR (MOM6 PR #6) that restores many of the error messages about using uninitialized control structures, but without requiring the use of pointers, that is a complement to this one. Further inquiry will be needed as to whether the various control structures inside of parent control structures should be made allocatable to avoid expanding the memory footprint of the model, but for now this is to be working in all tested cases.
This PR has passed TC testing and came close enough to passing pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/14135 (it would have passed if one omitted MOM_parameter_doc file had been updated to reflect changes from the previous commit.
marshallward
pushed a commit
that referenced
this pull request
Nov 25, 2021
Added a new variable, initialized, to the control structures of modules that had been testing for an allocated control structure to verify that it had been initialized before it was going to be used, and then duplicated the tests using this new variable. This was done to enable us to go ahead with MOM6 PR #5, which eliminated many of these checks when converting the control structures from pointers in the parent modules to elements that are always there, and then passing them as simple types instead of as pointers. If we decide that we do not need these tests after all, we can easily delete them, but until this is discussed, this commit avoids losing the messages, as it was easier to do it this way instead of trying to recreate them after they had been removed. All answers and output are bitwise identical.
This was referenced Jan 29, 2022
Hallberg-NOAA
added
the
refactor
marshallward
pushed a commit
that referenced
this pull request
Jul 2, 2022
* initial hooks for stochastic EOS modifications * remove debug statements * add documentation * Change ampltiude from 0.39 to sqrt(.39) * remove global_indexing logic from stoch_eos_init * switch to using MOM_random and add restart capability * update random sequence to update each each time-step * remove tseed0 from MOM_random (leftover from debugging) * Added necessary submodules and S^2, T^2 diagnostics to MOM_diagnostics * Added diagnostics for outputting variables related to the stochastic parameterization. * Diagnostics in MOM_PressureForce_FV updated for stochastic (rather than deterministic) Stanley SGS T variance parameterization. * Added parentheses for reproducibility. * Changed diagnostics to account for possible absence of stoch_eos_pattern in MOM_PressureForce_FV, when deterministic parameterization is on. * remove mom6_da_hooks and geoKdTree from pkg * add stochastic compoment to MOM_thickness_diffuse * fix array size declaration and post_data * Corrected indexing of loops in MOM_calc_varT * Changed how parameterization of SGS T variance (deterministic and stochastic) is switched on in PGF and thickness diffusion codes * Corrected a few typos * Cleaned up indices, redundant diagnostic, printing * Fixed diagnostic IDs * Fixed diagnostics typo * Corrected indices in calculation of tv%varT * Minor index fix * Corrected bug in pressure in Stanley diagnostics * Fixed whitespace error * Stoch eos clock (#5) *Added a clock for the Stanley parameterization Co-authored-by: jkenigson <jkenigso@gmail.com> * add halo update to random pattern * Update MOM_stoch_eos.F90 Fix bug for looping over compute domain (is -> isc etc.) * Avoid unnessary computations on halo (MOM_stoch_eos) and code clean-up (MOM_thickness_diffuse) * Removed halo updates before determ param calc * Update MOM_stoch_eos.F90 Removed unnecessary code * Bug - indices are transposed * Changed Stanley stochastic coefficient from exp(X) to exp(aX) (#9) * Changed Stanley stochastic coefficient from exp(X) to exp(aX) * Extra spaces removed * Stoch eos init fix (#10) * Don't bother calculating tv%varT if stanley_coeff<0 * Missing then added * Merge Ian Grooms Tvar Discretization (#11) * Update MOM_stoch_eos.F90 In progress updating stencil for$ | dx \times \nabla T|^2$ calculation * New discretization of |dx\circ\nablaT|^2 Co-authored-by: Ian Grooms <ian.grooms@colorado.edu> * Multiplied tvar%SGS by grid cell thickness ratio * Added limiter for tv%varT * Stoch eos ncar linear disc (#12) * Update MOM_stoch_eos.F90 In progress updating stencil for$ | dx \times \nabla T|^2$ calculation * New discretization of |dx\circ\nablaT|^2 * AR1 timescale land mask Adds land mask to the computation of the AR1 decorrelation time * Update dt in call to MOM_stoch_eos_run The call to `MOM_stoch_eos_run` (which time steps the noise) is from within `step_MOM_dynamics`. `step_MOM_dynamics` advances on time step `dt` (per line 957), but the noise is updated using `dt_thermo`. It seems more appropriate to update the noise using `dt`, since it gets called from within `step_MOM_dynamics`. * Fixed the units for r_sm_H * Remove vestigial declarations The variables `hl`, `Tl`, `mn_T`, `mn_T2`, and `r_sm_H` are no longer used, so I removed their declarations and an OMP private clause Co-authored-by: Ian Grooms <ian.grooms@colorado.edu> * Update MOM_thickness_diffuse.F90 Changed index for soft convention * Update CVMix-src * Ensure use_varT, etc., initialized * Don't register stanley diagnostics if scheme is off * Stanley density second derivs at h pts (#15) * Change discretization of Stanley correction (drho_dT_dT at h points) * Limit Stanley noise, shrink limiting value * Revert t variance discretization * Reverted variable declarations * Stanley scheme in mixed_layer_restrat, vert_fill in stoch_eos, code cleanup (#19) * Test Stanley EOS param in mixed_layer_restrat * Fix size of TS cov, S var in Stanley calculate_density calls * Test move stanley scheme initialization * Added missing openMP directives * Revert Stanley tvar discretization (#18) * Perform vertical filling in calculation of T variance * Variable declaration syntax error, remove scaling from get_param * Fix call to vert_fill_TS * Code cleanup, whitespace cleanup Co-authored-by: Jessica Kenigson <jessicak@cheyenne1.cheyenne.ucar.edu> * Use Stanley (2020) variance; scheme off at coast * Comment clean-up * Remove factor of 0.5 in Tvar * Don't calculate Stanley diagnostics on halo * Change start indices in stanley_density_1d * Stanley param in MOM_isopycnal_slopes (#22) Stanley param in MOM_isopycnal_slopes and thickness diffuse index fix * Set eady flag to true if use_stored_slopes is true * Cleanup, docs, whitespace * Docs and whitespace * Docs and whitespace * Docs and whitespace * Whitespace cleanup * Whitespace cleanup * Clean up whitespace * Docs cleanup * use_stanley * Update MOM_lateral_mixing_coeffs.F90 * Adds link to another TEOS10 module * Set Stanley off for testing * Line continuation Co-authored-by: Phil Pegion <38869668+pjpegion@users.noreply.github.com> Co-authored-by: Philip Pegion <Philip.Pegion@noaa.gov> Co-authored-by: Jessica Kenigson <jessicak@cheyenne6.cheyenne.ucar.edu> Co-authored-by: Jessica Kenigson <jessicak@cheyenne3.cheyenne.ucar.edu> Co-authored-by: jkenigson <jkenigso@gmail.com> Co-authored-by: jskenigson <jessica.kenigson@colorado.edu> Co-authored-by: Jessica Kenigson <jessicak@cheyenne1.cheyenne.ucar.edu> Co-authored-by: Jessica Kenigson <jessicak@cheyenne5.cheyenne.ucar.edu> Co-authored-by: Philip Pegion <ppegion@Philips-MacBook-Pro.local> Co-authored-by: Jessica Kenigson <jessicak@cheyenne4.cheyenne.ucar.edu>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces about 500 pointer declarations in MOM6 as either allocatables or locals.
This represents about 25% of the ~2000 total pointer allocation statements ("
pointer.*::) in the code. In some cases, a single statement may include many variables.Pointers which appeared in function declarations but were unused in the function were removed, resulting in some internal API changes.
Functions which no longer served any purpose, such as
<process>_endfunctions which only contained adeallocate(CS), have been removed.When appropriate, declarations such as
real, dimension(:,:), pointer :: xwere rewritten to use the more compact form,real, allocatable :: x(:,:).When possible
intent()directives were added to each declaration. Moving forward, we should assert that all function arguments useintent(). Where possible, I chose the most conservative option, although in many casesintent(inout)was required for some very trivial tasks such as flag or diagnostic updates, which may be better placed elsewhere.Many
if (.not. associated(CS)) call MOM_error(FATAL, ...)tests were removed, since such tests are no longer possible. Although this removes a small degree of defensive coding, many functions were only called once and such tests were rarely useful in practice.Where possible and appropriate,
associated()tests were replaced withallocated()tests.In a few cases, such as the re-allocation of an allocated variable, the tests are no longer necessary, since Fortran permits the double-allocation of a pointer but not an allocatable array.
When crucial decision-making relied on the behavior of an
associated()test, I generally left the pointer undisturbed. If I could deduce that the function was only called once, and the variable always existed, then I would remove the test. But such instances were rare.Many control structures were redefined as local variables inside of parent control structures, and are in many cases passed as stack variables to other functions. When these contain arrays, this was shown to relieve some performance bottlenecks in some previous work. However, I have done no profiling of this PR and cannot yet comment on its performance impact.
The SIS2 PR NOAA-GFDL/SIS2#159 is required for this PR to pass the GFDL regression test.
Although this has passed the GFDL regression suite, this has proven to be a very volatile exercise which has both discovered bugs and exposed otherwise mysterious behavior, so I think it's best to submit this partially complete version.