Refactor fates history output #143

bandre-ucar · 2016-10-26T19:48:53Z

Refactor the fates history output.

Refactor the fates history output to simplify the code, reduce duplication, cleanup
whitespace and rename variables and types in the hopes of clarifying their purpose and use.
The only fundamental change to the algorithm is the removal of the global-ish pointers to
data that caused runtime errors in nag. Summary of select changes:

Fates history types that are seen by the host via the hlm-fates interface are name-spaced
as FatesHistoryXXXMod.
Most history related classes are moved into separate modules.
Type bound procedures are used for initialization and common data manipulation routines.
The location of calls to access or manipulate history related data were changed when
possible to isolate operations and data types from the host. More work needs to be done.
Replace hardcoded strings and numbers with immutable parameters.
Use access routines to set and get some data needed by external objects.
Remove pointers to common data that essentially made that data global, and globally
mutable. Replace hard coded variables with an array, and store relevant array indices for
accessing the correct data. Pass common data into subroutines as intent in. The pointers
also resulted in 'dangling pointers' that caused runtime errors when compiled with
NAG (History output refactor broke nag #123).

Fixes: NAG dangling pointers issue, #123

User interface changes?: No

Code review: andre

Updated testing:

Test: SMS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest
Test baseline: 53bbb9d
Test status: pass

Test suite: ed - yellowstone gnu, intel, pgi
Test baseline: 53bbb9d
Test status: all tests pass

Test suite: ed - hobart nag
Test baseline: none, previous master did not run under nag.
Test status: all tests pass

Test suite: clm_shorts - yellowstone gnu, intel, pgi
Test baseline: clm4_5_12_r195
Test status: all tests pass

Original testing:

Test suite: ed - yellowstone gnu, intel, pgi
Test baseline: 44aac42
Test status: all tests pass

Test suite: ed - hobart nag
Test baseline: none
Test status: all tests pass

Test suite: clm_short - yellowstone gnu, intel, pgi
Test baseline: clm4_5_12_r195
Test status: all tests pass

Move the fates history dimensions class into a standalone module, along with the procedures for initialization and setting threads. Test suite: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 49733e8 Test namelist changes: none Test answer changes: bit for bit Test summary: pass

Refactor the fates history dimension kind struct, move it into its own module, create init function to remove copy-paste variable initialization. Test suite: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 49733e8 Test namelist changes: none Test answer changes: bit for bit Test summary: pass

Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass

Replace some hard coded strings with string parameters. Split a thread loop into two parts so allocation and assignment of data can be grouped together. Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass

Remove the pointers to the fates_history_dimensions stored in the iovar_dimension kind types. Replace them with integer indicies. indicies point into an array of dimension types. The indices - name mapping and dim array is stored in the top level fates history io object passed into subroutines as read only data as needed. Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass Test suite: ed - yellowstone gnu, intel, pgi Test baseline: 44aac42 Test status: all tests pass

Move some history initialization details out of the clm fates interface and into the history module. Convert another pointer into an allocatable. Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass

Test: ERS_D_Ld5.5x5_amazon.ICLM45ED.hobart_nag.clm-edTest Test baseline: none (master didn't compile and run on nag) Test status: pass lease enter the commit message for your changes. Lines starting

Create a small hobart specific test suite for testing ed with nag while dealing with some hobart specific limitations that prevent running the full ed test suite. Test suite: ed - hobart nag Test baseline: none, previous version didn't run Test status: all tests pass.

Rename the history interface file to have a consistent name other history related files. no source changes. Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass

Rename the history interface class and corresponding instance. Note: ammending commit, test was not a clean build, may not compile. Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass

Note: ammend commit, compilation errors with openmp on. Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass

error. Tests: ERP_D_Ld3_P15x2.f10_f10.ICLM45BGC.yellowstone_intel.clm-default ERP_D_Ld3_P15x2.f10_f10.ICLM45BGC.yellowstone_gnu.clm-default Test baseline: 44aac42 Test status: pass Test suite: ed - yellowstone gnu, intel, pgi Test baseline: 44aac42 Test status: all tests pass Test suite: ed - hobart nag Test baseline: none Test status: all tests pass Test suite: clm_short - yellowstone gnu, intel, pgi Test baseline: clm4_5_12_r195 Test status: all tests pass

ckoven · 2016-10-26T20:27:41Z

components/clm/src/ED/main/FatesHistoryInterfaceMod.F90

+     procedure, private :: define_history_vars
+     procedure, private :: set_history_var
+     procedure, private :: init_dim_kinds_maps
+     procedure, private :: set_dim_indicies


sorry, but i'm becoming more sensitive to typos in my old age. could we change set_dim_indicies to set_dim_indices?

Yep. I have limited time to work on ED for the next few days. I'd prefer to make all changes at once and only do one more testing cycle.

rgknox · 2016-10-28T20:52:56Z

components/clm/src/ED/main/FatesHistoryDimensionMod.F90

+    ! FIXME(bja, 2016-10) do these need to be strings, or can they be integer enumerations?
+    character(*), parameter :: patch_r8 = 'PA_R8'
+    character(*), parameter :: patch_ground_r8 = 'PA_GRND_R8'
+    character(*), parameter :: patch_class_pft_r8 = 'PA_SCPF_R8'


would recommend calling this patch_size_pft_r8

rgknox · 2016-10-28T20:53:22Z

components/clm/src/ED/main/FatesHistoryDimensionMod.F90

+    character(*), parameter :: patch_class_pft_r8 = 'PA_SCPF_R8'
+    character(*), parameter :: site_r8 = 'SI_R8'
+    character(*), parameter :: site_ground_r8 = 'SI_GRND_R8'
+    character(*), parameter :: site_class_pft_r8 = 'SI_SCPF_R8'


recommend: site_size_pft_r8

bandre-ucar · 2016-10-28T21:10:28Z

components/clm/src/ED/main/FatesHistoryDimensionMod.F90

@@ -0,0 +1,92 @@
+module FatesHistoryDimensionMod


@rgknox I kept the name of the module similar to the original derived type. Does it make more rename 'dimension' --> 'bounds'? That is more inline with the data and use....

I don't feel strongly either way. The patch, site, levgrnd and scpf are indeed the dimensions of the history arrays, but the critical functions involve setting the bounds of those dimensions.

rgknox · 2016-10-28T21:26:35Z

components/clm/src/ED/main/FatesHistoryVariableType.F90

+
+contains
+
+  subroutine Init(this, vname, units, long, use_default, &


@bandre-ucar, is it still possible to change the generic "Init()" subroutines changed to a more explicit naming convention? like here: Init_history_variable()
You mentioned to me offline that there are sometimes benefits regarding inheritance and method over-riding, but was curious if you felt the generic Init() name was still necessary.
I have a much easier time tracking through the code when the subroutine names are all unique.

I followed the convention that's been adopted by CLM. See WaterStateType, WaterFluxType, etc. CLM's convention is based on common OO practices, where waterstate_inst%Init() is the initialization method bound to this water state instance. If we do use polymorphism, then this approach remains consistent throughout the codebase. The approach you are proposing breaks down. While it may not be perfect and there are other approaches, it's commonly used, generally clear and consistent. I'm reluctant to deviate from this convention.

Would making the instance names more explicit work instead? More whitespace to clearly highlight the bound method? instead of hvar%Init(param1,param2) something like hist_var%Init( param1, param2, ... )

OK, lets keep it as is. Its not hard to track Init to the correct module or class.
As for changing instance names, I don't think changing the instance names will impact visibility for me personally.

rgknox · 2016-10-28T21:34:21Z

components/clm/src/ED/main/FatesHistoryVariableType.F90

+
+  ! =====================================================================================
+
+  subroutine GetBounds(this, thread, dim_bounds, dim_kinds, lb1, ub1, lb2, ub2)


nice job reducing the pointer to pointer-ness, retrieving dimension bounds from each variable seemed to be the most challenging.

rgknox · 2016-10-28T21:41:48Z

components/clm/src/ED/main/FatesHistoryVariableType.F90

+       this%int1d(lb1:ub1) = nint(this%flushval)
+    case default
+       write(fates_log(),*) 'fates history variable type undefined while flushing history variables'
+       stop


This reminds me that we need a graceful failure routines for FATES. I will make an issue if we don't have one.

rgknox · 2016-10-28T21:51:16Z

@bandre-ucar , my general feelings towards these changes are that they make a real nice improvement to our history output scheme.
Some open musings: I am wondering if FatesHistoryDimensionsMod could be expanded so that we could use it for an instantiation (or not) for restarts. Restarts will require one more dimension (cohort) and won't make use of the second dimension.
I hit the test button.

rgknox · 2016-11-01T21:18:09Z

@bandre-ucar, I don't have any other comments. I think the only thing I could nit pick is the name convetion of those scpf variables, instead of class, I recommend size.
But pending others comments I think we should resolve the conflicts and get it in. All tests on lawrencium against master 44aac42 passed.

bandre-ucar · 2016-11-01T21:39:15Z

@rgknox I was planning on addressing all your comments when I merged in the latest trunk. I just couldn't get to it yesterday and yellowstone is down today. Should be done tomorrow.

Test: SMS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 53bbb9d Test status: pass

bandre-ucar · 2016-11-02T22:02:28Z

I have a branch updated to master and have fixed the naming issues above. Retesting on yellowstone and nag. I will push an updated branch when tests are back.

Testing: Test: SMS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 53bbb9d Test status: pass Test suite: ed - yellowstone gnu, intel, pgi Test baseline: 53bbb9d Test status: all tests pass Test suite: ed - hobart nag Test baseline: none, previous master did not run under nag. Test status: all tests pass Test suite: clm_shorts - yellowstone gnu, intel, pgi Test baseline: clm4_5_12_r195 Test status: all tests pass

bandre-ucar · 2016-11-03T19:25:54Z

All tests passed on yellowstone and hobart.

rgknox · 2016-11-03T22:27:53Z

testing on lawrencium

rgknox · 2016-11-03T23:29:05Z

all tests pass, pulling in

bandre-ucar added 13 commits October 12, 2016 16:35

Merge branch andre-hist-io-refactor up to 44aac42

261aca9

Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass

Refactor fates history to class and module for hist variables.

719c030

Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass

clm-fates interface cleanup

f261c34

Replace some hard coded strings with string parameters. Split a thread loop into two parts so allocation and assignment of data can be grouped together. Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass

Refactor history output

7408f38

Move some history initialization details out of the clm fates interface and into the history module. Convert another pointer into an allocatable. Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass

NAG bugfix: break line that exceeds 132 characters.

a6f353d

Test: ERS_D_Ld5.5x5_amazon.ICLM45ED.hobart_nag.clm-edTest Test baseline: none (master didn't compile and run on nag) Test status: pass lease enter the commit message for your changes. Lines starting

Rename HistoryIOMod.F90 -> FatesHistoryInterfaceMod.F90

31572b4

Rename the history interface file to have a consistent name other history related files. no source changes. Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass

Code cleanup for history names and whitespace

1622c86

Note: ammend commit, compilation errors with openmp on. Test: ERS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 44aac42 Test status: pass

ckoven reviewed Oct 26, 2016

View reviewed changes

rgknox reviewed Oct 28, 2016

View reviewed changes

bandre-ucar commented Oct 28, 2016

View reviewed changes

rgknox reviewed Oct 28, 2016

View reviewed changes

Merge branch 'master' into andre-hist-io-refactor

f49e7a5

Test: SMS_D_Ld5.f10_f10.ICLM45ED.yellowstone_intel.clm-edTest Test baseline: 53bbb9d Test status: pass

rgknox merged commit 2ac7960 into NGEET:master Nov 3, 2016

bandre-ucar deleted the andre-hist-io-refactor branch February 24, 2017 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor fates history output #143

Refactor fates history output #143

bandre-ucar commented Oct 26, 2016 •

edited

Loading

ckoven Oct 26, 2016

bandre-ucar Oct 26, 2016

bandre-ucar Nov 2, 2016

rgknox Oct 28, 2016

bandre-ucar Nov 2, 2016

rgknox Oct 28, 2016

bandre-ucar Nov 2, 2016

bandre-ucar Oct 28, 2016

rgknox Oct 28, 2016

rgknox Oct 28, 2016

bandre-ucar Oct 28, 2016

rgknox Oct 28, 2016

rgknox Oct 28, 2016

rgknox Oct 28, 2016

rgknox commented Oct 28, 2016

rgknox commented Nov 1, 2016

bandre-ucar commented Nov 1, 2016

bandre-ucar commented Nov 2, 2016

bandre-ucar commented Nov 3, 2016

rgknox commented Nov 3, 2016

rgknox commented Nov 3, 2016


		contains

		subroutine Init(this, vname, units, long, use_default, &


		! =====================================================================================

		subroutine GetBounds(this, thread, dim_bounds, dim_kinds, lb1, ub1, lb2, ub2)

Refactor fates history output #143

Refactor fates history output #143

Conversation

bandre-ucar commented Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgknox commented Oct 28, 2016

rgknox commented Nov 1, 2016

bandre-ucar commented Nov 1, 2016

bandre-ucar commented Nov 2, 2016

bandre-ucar commented Nov 3, 2016

rgknox commented Nov 3, 2016

rgknox commented Nov 3, 2016

bandre-ucar commented Oct 26, 2016 •

edited

Loading