Adding the fms_diag_object container. #867

ngs333 · 2021-12-06T22:40:03Z

Description

This PR is for the initial commit of the fms_diag_object_container. Includes the underlying linked_list library,
some changes to diag_manger to initialize the container and to use the container upon field registration, and related Makefile.am changes.

Note that the project had decided on 11/29 that the singleton diag_object_container would be placed in the
diag_data module. This was not possible because it introduced circular dependecies between the
diag_data and diag_object modules, and so the singleton was placed in the diag_manager module.

Fixes # (issue)
This is in support of the new diag manager.

How Has This Been Tested?
The code was build with gcc 9.3. "make check" passes the same number if tests (31/33) with as
without this item. Tests for the new functionality is a TBD.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules
New check tests, if applicable, are included
make distcheck passes

linked_list library, some changes to diag_manger to initialize the container and to use the container upon field registration, related Makefile.am changes.

…system.

thomas-robinson

Please add some tests to the code and address the comments I've made.

thomas-robinson · 2021-12-07T16:10:33Z

diag_manager/fms_doubly_linked_list.F90

+!! \description A double linked list module that is rougly a fortran translation of the C++
+!! boubly linked list from the book  ``Data Structures And Algorithm Analysis in C++", 3rd Edition,
+!!  by Mark Allen Weiss
+module fms_doubly_linked_list_mod


If this is specific for the diag manager, then it should be named fms_diag_doubly_linked_list_mod. If you expect that it could be used more generally, then you should create a new folder in FMS with it's own Makefile.

This needs a unit test. Can you come up with a test or tests for this?

thomas-robinson · 2021-12-07T16:11:13Z

diag_manager/Makefile.am

-  fms_diag_yaml_object.F90
+  fms_diag_yaml_object.F90 \
+  fms_diag_object_container.F90 \
+  fms_doubly_linked_list.F90


See other comment about naming this module/file

thomas-robinson · 2021-12-07T16:11:56Z

diag_manager/diag_manager.F90

@@ -223,9 +223,11 @@ MODULE diag_manager_mod
       & use_mpp_io, use_modern_diag
  USE diag_data_mod, ONLY:  fileobj, fileobjU, fnum_for_domain, fileobjND
  USE diag_table_mod, ONLY: parse_diag_table
+  !!USE diag_data_mod, ONLY : the_diag_object_container


Delete this. No need to keep it around if it's not being used

thomas-robinson · 2021-12-07T16:13:25Z

diag_manager/diag_manager.F90

@@ -261,6 +263,8 @@ MODULE diag_manager_mod

  type(time_type) :: Time_end

+  TYPE(fms_diag_object_container), ALLOCATABLE :: the_diag_object_container


The naming conventions for FMS have this line as

TYPE(fms_diag_object_container_type), ALLOCATABLE :: diag_object_container

Where the type has _type at the end

This variable should be documented

Refactored in VS Code to added the _type.
There are some classes that are are missing the "_type", or are following another convention.
Let me know if you want to refactor those also.
BTW, is it possible to use just "_t" and not "_type" ?

I just reread the style guide: https://github.com/NOAA-GFDL/FMS/blob/main/CODE_STYLE.md#derived-types . It looks like we all have some updating to do. Pretty much everything needs to be updated, although I will bring this up at the FMS code meeting next week.

thomas-robinson · 2021-12-07T16:22:41Z

diag_manager/diag_manager.F90

+    class(fms_diag_object), allocatable , target ::  a_diag_obj
+    type(fms_diag_object), pointer :: diag_obj_ptr


These variables should be documented

The initial pointer should point to null

thomas-robinson · 2021-12-07T18:19:04Z

diag_manager/fms_doubly_linked_list.F90

+    class(literator), intent(in) :: this
+    class(*),  pointer  :: rd


Variables need to be documented in doxygen style

thomas-robinson · 2021-12-07T18:19:15Z

diag_manager/fms_doubly_linked_list.F90

+  !! Iterate over all the nodes, remove them and deallocate the client data
+  !! that the node was holding.
+  subroutine clear_all( this  )
+    class(linked_list), intent(inout) :: this
+    class(node), pointer :: nd
+    class(literator), allocatable :: iter
+    class(*),  pointer  :: pdata


The function needs a doxygen description.
Variables need to be documented in doxygen style

thomas-robinson · 2021-12-07T18:19:26Z

diag_manager/fms_doubly_linked_list.F90

+  ! A destructor that deallocates every node and each nodes data element.
+    subroutine destructor(this)
+    type(linked_list ) :: this  !Note for destructors its needs to be type and not class!


The function needs a doxygen description.
Variables need to be documented in doxygen style

thomas-robinson · 2021-12-07T18:20:05Z

diag_manager/fms_doubly_linked_list.F90

+      iter =  this%remove(nd)
+      pdata => iter%get()
+      if (associated(pdata) .eqv. .false.) then
+        print *, "clear all. pdata is not associated"


Use mpp_error or some other routine to print

thomas-robinson · 2021-12-07T18:21:30Z

diag_manager/fms_doubly_linked_list.F90

+    nd%data => d
+    nd%prev => t_nd%prev
+    nd%next => t_nd
+    t_nd%prev%next => nd
+    t_nd%prev => nd


Can you give some description of what's happening here?

Done. Added explanation with diagrams.

on 12/7/01. Mostly documentation, logging, and type name improvements.

ngs333 · 2021-12-08T02:56:52Z

@thomas-robinson
Thanks for the great review. I made the changes relating to comments, type names, and error
logging. Some work still todo:

I still havent decided on where to put the fms_doubly_linked_list.
I have unit tests
in the old diag_manager2 prototype project and will move them over soon.
Not sure about the interface issue you raise in ( Adding the fms_diag_object container. #867 (comment) ) . Will investigate later.

ngs333 · 2021-12-08T03:01:18Z

@ngs333
TODO: investigate fortran class access controls. Only end client interfaces public?

…low convention and documentation.

ngs333 · 2021-12-09T04:14:56Z

@thomas-robinson
The unit test was added.

ngs333 · 2021-12-09T17:16:49Z

@thomas-robinson , @uramirez
Just finished cleaning up the unit test for the diag object container.

ngs333 · 2021-12-09T17:40:39Z

@bensonr , @thomas-robinson , @uramirez8707
FYI : I had to change the unfinished/prototype function is_filed_type_null() in file fms_diag_yaml.F90.
This is because the way it was prototyped (always returning .false.) would not allow the creation of a diag object
with a non-zero ID. Diag objects with varying ids are needed to test. BTW, I added a "TODO:" in front of the prototype funciton.

…tandards

ngs333 · 2021-12-13T22:06:22Z

@thomas-robinson , @uramirez8707 , @GFDL-Eric
I just did another update, hopefully the last before merge. I builds with cmake and gnu tools.
The parser and yaml related files were added to the CMakeList.txt.

There is a unit test for fms_diag_object_container but not for fms_diag_dlinked_list - the former is is wrapper over the latter and everythig is tested as is.

FInally I did not change the constructors as per the review. I hope this can be delaed until we establish a convention.

compile on Intel.

… updates.

…eral in several types. Improved several comments.

uramirez8707 · 2021-12-21T17:54:50Z

.vscode/launch.json

@@ -0,0 +1,27 @@
+{


I think you didn't mean to commit this file.

@uramirez8707
Done. Thanks.
We shoudl consider putting a good one in.

uramirez8707 · 2021-12-21T17:55:10Z

.vscode/tasks.json

@@ -0,0 +1,39 @@
+{


I think you didn't mean to commit this file.

Done. Thanks.
We shoudl consider putting a good one in.

ngs333 · 2021-12-21T18:05:35Z

@thomas-robinson , @uramirez8707
In the last half hour I pushed a large update. Includes a test for the linked list,
improvments or additions ot/of many comments, and some minor code cleanup.

diag_manager/diag_manager.F90

uramirez8707 · 2021-12-21T18:10:27Z

diag_manager/diag_manager.F90

+      !! Create a diag object, initialize it with the registered data, and insert
+      !! it ino the diag_obj_container singleton.
+
+      allocate( diag_obj )


Where is this deallocated?

Contents of the container are deallocated by the container.
Either by the container destructor or the user calling the clear() function.

thomas-robinson

There is a comment handing about an endif on line 614 of diag_manager.F90 that should be deleted I think?
In fms_diag_object_container.F90, line 59 and 73 have a !! when I think it shoul be a !<. Please fix these.

thomas-robinson · 2022-01-04T14:55:36Z

diag_manager/diag_manager.F90

    endif

+    !! ***  ERROR MISSING ENDIF ???  ***  endif


What is this comment about?

Its a bad comment. Deleted.

thomas-robinson · 2022-01-04T15:13:47Z

diag_manager/fms_diag_object_container.F90

+   !!  This version does not enforce uniqueness of ID keys (I.e. it is not a set).
+   !!
+   type, public:: FmsDiagObjectContainer_t
+      TYPE (FmsDlList_t), ALLOCATABLE :: the_linked_list !!This version based on the FDS linked_list.


This still need to be corrected

thomas-robinson · 2022-01-04T15:14:11Z

diag_manager/fms_diag_object_container.F90

+
+   !> \brief Iterator used to traverse the objects of the container.
+   type, public :: FmsDiagObjIterator_t
+      type(FmsDllIterator_t) :: liter !!this version based on the FDS linked_list


This also needs to be corrected

thomas-robinson · 2022-01-04T15:21:05Z

@ngs333 You have some unchecked boxes at the top.

My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules
New check tests, if applicable, are included
make distcheck passes

Are you generating new warnings with this code? And are the new tests passing? All of the boxes should be checked.

ngs333 · 2022-01-04T17:25:29Z

@thomas-robinson I think I made the requested changes. I also pulled and merged the upstream branch into mine.

diag_manager/fms_diag_dlinked_list.F90

rem1776 · 2022-01-04T18:40:54Z

diag_manager/fms_diag_object.F90

@@ -158,6 +159,7 @@ subroutine diag_obj_init(ob)
 end subroutine diag_obj_init
 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 !> \description Fills in and allocates (when necessary) the values in the diagnostic object
+!


This added line should be removed, since it's only 1 ! doxygen won't read the line and it probably won't match the description to the routine.

Initial commit of the fms_diag_object_container. Includes the underlying

1dc9699

linked_list library, some changes to diag_manger to initialize the container and to use the container upon field registration, related Makefile.am changes.

ngs333 requested a review from thomas-robinson as a code owner December 6, 2021 22:40

ngs333 requested review from uramirez8707, thomas-robinson, GFDL-Eric and bensonr and removed request for thomas-robinson December 6, 2021 22:40

ngs333 assigned thomas-robinson Dec 6, 2021

Modified diag object iterator to fix casting compilation error on CI …

02c9f53

…system.

bensonr approved these changes Dec 7, 2021

View reviewed changes

thomas-robinson requested changes Dec 7, 2021

View reviewed changes

Initial modificationss in response to a review by Tom Robinsom

6275892

on 12/7/01. Mostly documentation, logging, and type name improvements.

Miguel Zuniga added 2 commits December 8, 2021 13:29

Experimenting with documentation annotations.

5da33c2

Added test of fms_diag_object_container class. Further changes to fol…

3ec00fc

…low convention and documentation.

Miguel Zuniga added 2 commits December 9, 2021 11:01

Corrected script calling unit test. Added todo in fms_diag_yaml.F90.

a5ed9d4

Cleaned up test of container.

687aac6

Addded a "TODO:" to fms_diag_yaml.F90, function is filed type null.

7044c9f

Miguel Zuniga added 5 commits December 13, 2021 15:18

Modified new files and unit test for further compliance with coding s…

df06cc7

…tandards

Renamed the linked list mod. Corrected CMakeList.txtx and a Makefile.am.

8984a3b

Renamed linked list mod. Changed a Makefile.am.

5b398ea

Fixed CMakeLists.txt.

d826d8c

Mods to CMakeLIst.txt. Includes adding parser dir files.

d9265fa

Fixing typo in CMakeLists.txt

63c201d

Miguel Zuniga added 3 commits December 17, 2021 14:28

Five chages of list node instance declarations from class to node to

7bb5c07

compile on Intel.

One change from type to class for an interator instance. Some comment…

47814e8

… updates.

Added test_diag_dlinked_list.F90. Modified memeber data/access in sev…

025ccfd

…eral in several types. Improved several comments.

ngs333 requested review from rem1776 and wrongkindofdoctor as code owners December 21, 2021 17:34

uramirez8707 reviewed Dec 21, 2021

View reviewed changes

Miguel Zuniga added 2 commits December 21, 2021 12:59

Improved or added several comments.

64ca1fa

Removed vs code related files.

571041c

uramirez8707 reviewed Dec 21, 2021

View reviewed changes

diag_manager/diag_manager.F90 Show resolved Hide resolved

uramirez8707 reviewed Dec 21, 2021

View reviewed changes

Miguel Zuniga added 3 commits December 22, 2021 00:03

Removing extraneous return statement in test_diag_dlinked_list.F90.

cb2530e

Improved test_diag_dlinked_list.F90, both code and comments.

77a5960

Merge remote-tracking branch 'upstream/dmUpdate' into dmUpdate

c00b6a3

uramirez8707 approved these changes Jan 3, 2022

View reviewed changes

Removed duplicate yaml_parser.F90 from CMakeLists.txt.

bbef9c6

thomas-robinson requested changes Jan 4, 2022

View reviewed changes

Miguel Zuniga and others added 3 commits January 4, 2022 11:55

Incorporates changes based on Tom R's latest review.

415a38a

Merge branch 'NOAA-GFDL:dmUpdate' into dmUpdate

8682eda

Merge branch 'dmUpdate' of https://github.com/ngs333/FMS into dmUpdate

1fb5a63

rem1776 reviewed Jan 4, 2022

View reviewed changes

Includes some doxygen related changes requested by Ryan M.

01dec84

thomas-robinson approved these changes Jan 4, 2022

View reviewed changes

thomas-robinson merged commit 60c5e12 into NOAA-GFDL:dmUpdate Jan 4, 2022

rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024

feat: Add the fms_diag_object container. (NOAA-GFDL#867)

e08b6d0

rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024

feat: Add the fms_diag_object container. (NOAA-GFDL#867)

17de21c

rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024

feat: Add the fms_diag_object container. (NOAA-GFDL#867)

1edbc32

		@@ -261,6 +263,8 @@ MODULE diag_manager_mod

		type(time_type) :: Time_end

		TYPE(fms_diag_object_container), ALLOCATABLE :: the_diag_object_container

		class(fms_diag_object), allocatable , target :: a_diag_obj
		type(fms_diag_object), pointer :: diag_obj_ptr

Adding the fms_diag_object container. #867

Adding the fms_diag_object container. #867

Conversation

ngs333 commented Dec 6, 2021 • edited Loading

thomas-robinson left a 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

ngs333 Dec 7, 2021 • 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

Choose a reason for hiding this comment

ngs333 commented Dec 8, 2021 • edited Loading

ngs333 commented Dec 8, 2021

ngs333 commented Dec 9, 2021

ngs333 commented Dec 9, 2021

ngs333 commented Dec 9, 2021

ngs333 commented Dec 13, 2021

uramirez8707 Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uramirez8707 Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngs333 commented Dec 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomas-robinson left a 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

thomas-robinson commented Jan 4, 2022

ngs333 commented Jan 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngs333 commented Dec 6, 2021 •

edited

Loading

ngs333 Dec 7, 2021 •

edited

Loading

ngs333 commented Dec 8, 2021 •

edited

Loading

uramirez8707 Dec 21, 2021 •

edited

Loading

uramirez8707 Dec 21, 2021 •

edited

Loading