Dm get fns #885

thomas-robinson · 2022-01-11T19:14:21Z

Description

Uses a class(*) variable for missing_value and data_RANGE
Fills in CMOR_MISSING_VALUE for missing_value if none is given
Adds rountines for fmsDiagObject_type

Fixes #884

How Has This Been Tested?
make with intel compilers

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

Fills in CMOR_MISSING_VALUE for missing_value if none is given Adds rountines for fmsDiagObject_type

diag_manager/fms_diag_object.F90

ngs333

@thomas-robinson
I added several comments but you may first want to read the longer one for
function "get_metadata". If you want we can discuss these in a meeting, etc.

ngs333 · 2022-01-12T16:34:23Z

diag_manager/fms_diag_object.F90

@@ -396,17 +422,323 @@ function diag_obj_is_static (obj) result (rslt)
    rslt = obj%static


This whole function looks wierd to me. is member "static" and id ?
This mayneed a "!TODO:"

diag_manager/fms_diag_object.F90

ngs333 · 2022-01-12T17:01:33Z

diag_manager/fms_diag_object.F90

+end function get_metadata
+!> @brief Gets static
+!! @return true if the variable is static
+function get_static (obj) &


What you have works, but we should discuss if its a good idea to follow a convention that adds
a function ( doesn;t get rig of the get_satic, but adds another ) named is_static
It is used like

if (obj%is_static() ) then

In the end, waht you ahve passes the litmus test that these two statements be
equivalent (if static had been public)

s = obj%static s = obj%get_static()

(this is because staic is another intrisic (logical) like int, real etc.

ngs333 · 2022-01-12T17:09:42Z

diag_manager/fms_diag_object.F90

+end function get_static
+!> @brief Gets regisetered
+!! @return true if the object is registered
+function get_registered (obj) &


See get_static comment. Do we want a user to have if (obj%is_registered() ?

Yes, we WANT the user to check if something is allocated or registered. What happens if they don't?

ngs333 · 2022-01-12T17:17:26Z

diag_manager/fms_diag_object.F90

@@ -35,7 +35,7 @@ module fms_diag_object_mod
     integer, allocatable, private                    :: diag_id           !< unique id for varable
     class(FmsNetcdfFile_t), dimension (:), pointer   :: fileob => NULL()  !< A pointer to all of the
                                                                           !! file objects for this variable
-     character(len=:), allocatable, dimension(:)      :: metadata          !< metedata for the variable
+     character(len=:), allocatable, dimension(:)      :: metadata          !< metadata for the variable
     logical, private                                 :: static            !< true is this is a static var
     logical, allocatable, private                    :: registered        !< true when registered


I think its less complex and more efficient if the intrinsic primitives (like logical, integer, real) are not allocatable
and have a default set in the type definiition. E.g. like this

logical, private :: registered => .false.

it would help to add functions like logical is_registered() (see comments below). Probably, type fms_dia_object
may nedd one or more initialize functions that the constructor calls.

There was a reason I did it this way, but I can't remember now. I think it was so that objects could be unregistered and without deallocating the object. Also, I think some of the optional variables were left allocatable so that someone could check if they were allocated first.

if (obj%allocated_metadata()) metadata = obj%get_metadata()

I actually just finished the allocated functions. We should still discuss.

ngs333 · 2022-01-12T17:28:41Z

diag_manager/fms_diag_object.F90

+!! @return the variable name
+function get_varname (obj) &
+result(rslt)
+     class (fmsDiagObject_type), intent(in) :: obj !< diag object


I guess in this case the getter litterally returns the member field (seems to pass the litmus test). But I will read (or as) what exatly does the assignemnt (=) do. Hope we dont have to look at assemberr :-)

ngs333 · 2022-01-12T17:29:43Z

diag_manager/fms_diag_object.F90

+!> @brief Gets longname
+!! @return the variable long name or a single string if there is no long name
+function get_longname (obj) &
+result(rslt)


And in this case we dont follow the litmus test with the else = " " statement. Too confusing to not follow one rule for such getters. BTW, One possibility is again with the default values in the type definition. Where
fields like longName is initialized (with =>) to "" or " " or some class or saystem default.

I added a varaible called diag_null_sring that is a parameter and has the value " ". This can be used in if statments

if (obj%get_longname() .ne. diag_null_string) longname = obj%get_longname()

ngs333 · 2022-01-12T17:39:27Z

diag_manager/fms_diag_object.F90

+     procedure :: get_interp_method
+     procedure :: get_frequency
+     procedure :: get_output_units
+     procedure :: get_t


Wat is "t" ? Is this a good name? Are there comments somewhere?

I think it was something that was temporarily being used. It should probably get deleted.

I will remove it in a later PR.

diag_manager/fms_diag_object.F90

Makes all integer get functions return diag_null if the variable is not allocated

thomas-robinson added 4 commits January 5, 2022 12:07

Changes fms_diag_object to fmsDiagObject_type

6d76777

Removes the multi-dimensional type extentions that will no be used

0790d64

Merge branch 'dmUpdate' into dmGetFns

f7345f2

Uses a class(*) variable for missing_value and data_RANGE

8e4c664

Fills in CMOR_MISSING_VALUE for missing_value if none is given Adds rountines for fmsDiagObject_type

thomas-robinson requested review from ngs333 and uramirez8707 January 11, 2022 19:14

thomas-robinson changed the base branch from main to dmUpdate January 11, 2022 19:14

uramirez8707 requested changes Jan 11, 2022

View reviewed changes

diag_manager/fms_diag_object.F90 Outdated Show resolved Hide resolved

diag_manager/fms_diag_object.F90 Outdated Show resolved Hide resolved

thomas-robinson added 2 commits January 12, 2022 08:11

Puts result for get_* functions on its own line in diag_object

10e8a24

Adds brief doxygen to get_* functions in diag_object

f20ab8d

uramirez8707 approved these changes Jan 12, 2022

View reviewed changes

Merge branch 'dmUpdate' of github.com:NOAA-GFDL/FMS into dmGetFns

9958903

ngs333 suggested changes Jan 12, 2022

View reviewed changes

thomas-robinson added 3 commits January 14, 2022 10:26

Updates comments on get functions in diag object

20828c8

Makes all integer get functions return diag_null if the variable is not allocated

Sets up parmeter diag_null_string as a single empty space

2dd3516

Adds is_ functions for logical variables in diag object

2b99316

ngs333 approved these changes Jan 31, 2022

View reviewed changes

thomas-robinson merged commit 08205c6 into NOAA-GFDL:dmUpdate Feb 7, 2022

thomas-robinson deleted the dmGetFns branch August 17, 2022 12:49

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

feat: add getter functions to fms_diag_object (NOAA-GFDL#885)

0df61fc

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

feat: add getter functions to fms_diag_object (NOAA-GFDL#885)

eb09096

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

feat: add getter functions to fms_diag_object (NOAA-GFDL#885)

b349abf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dm get fns #885

Dm get fns #885

thomas-robinson commented Jan 11, 2022 •

edited

Loading

ngs333 left a comment

ngs333 Jan 12, 2022

ngs333 Jan 12, 2022

ngs333 Jan 12, 2022

thomas-robinson Jan 14, 2022

ngs333 Jan 12, 2022

thomas-robinson Jan 13, 2022

ngs333 Jan 12, 2022

ngs333 Jan 12, 2022

thomas-robinson Jan 14, 2022

ngs333 Jan 12, 2022

thomas-robinson Jan 13, 2022

thomas-robinson Jan 14, 2022

		@@ -396,17 +422,323 @@ function diag_obj_is_static (obj) result (rslt)
		rslt = obj%static

Dm get fns #885

Dm get fns #885

Conversation

thomas-robinson commented Jan 11, 2022 • edited Loading

ngs333 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

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 11, 2022 •

edited

Loading