diag_axis io #982

uramirez8707 · 2022-06-01T21:52:18Z

Description
Adds procedure to the axis_object that write the axis metadata and the axis data.
Adds a subroutine to determine the fms2_io fileobj to use

Fixes # (issue)

How Has This Been Tested?
CI

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

thomas-robinson · 2022-06-02T11:07:33Z

diag_manager/fms_diag_axis_object.F90

@@ -85,6 +87,7 @@ module fms_diag_axis_object_mod
     CHARACTER(len=:),   ALLOCATABLE, private :: long_name       !< Long_name attribute of the axis
     CHARACTER(len=1)               , private :: cart_name       !< Cartesian name "X", "Y", "Z", "T", "U", "N"
     CLASS(*),           ALLOCATABLE, private :: axis_data(:)    !< Data of the axis
+     CHARACTER(len=:),   ALLOCATABLE, private :: data_type       !< The type of the axis_data


Is this real4, real8, integer4, etc? You should put the possible values.

I also think you should rename it to type_of_data because _type is for type definitions.

thomas-robinson · 2022-06-02T11:08:10Z

diag_manager/fms_diag_axis_object.F90

@@ -99,17 +102,17 @@ module fms_diag_axis_object_mod
     TYPE(diag_atttype),allocatable , private :: attributes(:)   !< Array to hold user definable attributes
     INTEGER                        , private :: num_attributes  !< Number of defined attibutes
     INTEGER                        , private :: domain_position !< The position in the doman (NORTH, EAST or CENTER)
+     INTEGER                        , private :: fileobj_type    !< The fileobj to use for variables in this axis


I think you should mention the possible values here in the comments.
Also, shouldn't this be named file_domain or axis_domain? This is telling you information about the domain. You aren't getting any information about the file object here, and the axis shouldn't have any information about the file. I understand this information is needed for the file object, but that should be handled at a file level, not at the axis level.

I don't think it's a good idea to add _type to the end of a variable because the style guide reserves that suffix for type definitions.

thomas-robinson · 2022-06-02T11:10:01Z

diag_manager/fms_diag_axis_object.F90

@@ -154,14 +157,17 @@ subroutine register_diag_axis_obj(obj, axis_name, axis_data, units, cart_name, l
    type is (real(kind=r8_kind))
      allocate(real(kind=r8_kind) :: obj%axis_data(size(axis_data)))
      obj%axis_data = axis_data
+      obj%data_type = "double" !< This is what fms2_io expects in the register_field call


My favorite part about this is that you had to put this comment in because you knew I would see it and comment about using "double" instead of something like "real_kind8". There was a very loud sigh.

thomas-robinson · 2022-06-02T11:33:41Z

diag_manager/fms_diag_axis_object.F90

+      call register_variable_attribute(fileobj, axis_name, "units", obj%units, str_len=len_trim(obj%units))
+
+    select case (obj%direction)
+    case (-1)


This needs to be a parameterized integer

thomas-robinson · 2022-06-02T11:33:47Z

diag_manager/fms_diag_axis_object.F90

+    select case (obj%direction)
+    case (-1)
+      call register_variable_attribute(fileobj, axis_name, "positive", "up", str_len=2)
+    case (1)


This needs to be a parameterized integer

thomas-robinson · 2022-06-02T11:52:10Z

diag_manager/fms_diag_axis_object.F90

+    class(FmsNetcdfFile_t), INTENT(INOUT) :: fileobj   !< Fms2_io fileobj to write the data to
+    integer, OPTIONAL,      INTENT(IN)    :: sub_axis_id !< ID of the sub_axis, if it exists
+
+    character(len=:), ALLOCATABLE :: axis_name !< Name of the axis


See comment below. Remove.

thomas-robinson · 2022-06-02T11:52:50Z

diag_manager/fms_diag_axis_object.F90

+  end subroutine write_axis_metadata
+
+  !> @brief Write the axis data to an open fileobj
+  subroutine write_axis_data(obj, fileobj, sub_axis_id)


This also needs a routine to call it from the file object level.

thomas-robinson · 2022-06-02T11:57:53Z

diag_manager/fms_diag_axis_object.F90

+        !< If they are different domains, one of them can be NO_DOMAIN
+        !! i.e a variable can have axis that are domain decomposed (x,y) and an axis that isn't (z)
+        if (fileobj_type .eq. NO_DOMAIN .or. axis_obj(j)%fileobj_type .eq. NO_DOMAIN ) then
+          !< Update the filobj_type and domain, if needed


typo: fileobj

thomas-robinson · 2022-06-02T11:58:37Z

diag_manager/fms_diag_axis_object.F90

+  !!        and return the domain
+  subroutine determine_the_fileobj_type(axis_id, fileobj_type, domain, var_name)
+    integer,                      INTENT(IN)  :: axis_id(:)    !< Array of axis ids
+    integer,                      INTENT(OUT) :: fileobj_type  !< fileobj_type to use


This is another case of the _type suffix on a variable that should be avoided.

thomas-robinson · 2022-06-02T12:15:24Z

diag_manager/fms_diag_axis_object.F90

+
+  !> @brief Loop through a variable's axis_id to determine and set the fms2_io filobj type to use
+  !!        and return the domain
+  subroutine determine_the_fileobj_type(axis_id, fileobj_type, domain, var_name)


Where is this being called?

Why is domain a pointer? I'm not 100% sure it's thread safe because of the pointer (I think it is thread safe, but I am just not totally positive).

The description says "to determine and set the fms2_io filobj type to use" but it uses axis_obj(j)%fileobj_type which is already set. So nothing is being set here.

This doesn't return a fileobj type; it returns an integer. If it didn't have the domain pointer as an intent (out) argument, this could be a pure integer function instead of a subroutine.

Where is this being called?

This is going to be called in register_diag_field, the field has an array of axis ids, this function is going to return the domain and the "fileobj_type" (the type of fms2_io fileobj the variable needs). This then has to be passed on to the diag_files. so we can know what fms2_io obj to open and we have the domain to open the file with.
I think I should rename "fileobj_type" to "type_of_domain"

Why is domain a pointer? I'm not 100% sure it's thread safe because of the pointer (I think it is thread safe, but I am just not totally positive).

This is going to be called the init, so there is no openmp(?). If it is not a pointer, then it has to be a copy ...

The description says "to determine and set the fms2_io filobj type to use" but it uses axis_obj(j)%fileobj_type which is already set. So nothing is being set here.

what if it says "to determine and return the domain and domain_type to use"

OK, so what happens to this domain pointer? Is the domain pointer you pass in part of the diag_object? I think if you had the actual call to this routine somewhere (even commented out for now), it would be more helpful for me to understand it.

Yes the domain_pointer is part of the diag_object

This is part of the next steps
f395714

I think we finally have all pieces for register_diag_field to be completely done.

…irection

uramirez8707 · 2022-06-02T15:40:51Z

@thomas-robinson I think all your comments have been addressed.

thomas-robinson · 2022-06-03T11:43:28Z

diag_manager/fms_diag_axis_object.F90

@@ -384,6 +484,38 @@ subroutine check_if_valid_edges(edges)
       call mpp_error(FATAL, "diag_axit_init: The edge axis has not been defined. "&
                             "Call diag_axis_init for the edge axis first")
  end subroutine check_if_valid_edges
+
+  !> @brief Loop through a variable's axis_id to determine and return the domain type and domain to use


I still think this description is lacking. The \brief describes what this subroutine does, but there is no explanation for it's use. I think with the next update, you should have a \description instead of \brief that explains why you use this. In the larger plan, this routine is necessary. If you're just looking at this module, you will scratch your head and wonder why this exists.

* Add the io procedures for the axis * add sub_axis info to the write_axis_metadata and write_data

uramirez8707 added 2 commits June 1, 2022 15:28

Add the io procedures for the axis

e51a1ce

add sub_axis info to the write_axis_metadata and write_data

99c41aa

uramirez8707 requested a review from thomas-robinson as a code owner June 1, 2022 21:52

uramirez8707 mentioned this pull request Jun 1, 2022

Diag_axis object missing work #976

Closed

6 tasks

thomas-robinson requested changes Jun 2, 2022

View reviewed changes

uramirez8707 added 3 commits June 2, 2022 11:33

renames some variables, adds more documentation, and parameters for d…

0f005b0

…irection

fix a typo and adds the use diag_data statements

5a8ddd7

one last typo

9b0c46a

Add TO DOs related to the diag_axis to the diag_obj

f395714

thomas-robinson approved these changes Jun 3, 2022

View reviewed changes

thomas-robinson merged commit 67f97ce into NOAA-GFDL:dmUpdate Jun 3, 2022

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

diag_axis io (NOAA-GFDL#982)

ac81cfe

* Add the io procedures for the axis * add sub_axis info to the write_axis_metadata and write_data

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

diag_axis io (NOAA-GFDL#982)

839e4cb

* Add the io procedures for the axis * add sub_axis info to the write_axis_metadata and write_data

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

feat: modern diag add io routines for diag_axis object (NOAA-GFDL#982)

a6b5f2f

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

feat: modern diag add io routines for diag_axis object (NOAA-GFDL#982)

791c787

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

feat: modern diag add io routines for diag_axis object (NOAA-GFDL#982)

0b90ed2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

diag_axis io #982

diag_axis io #982

uramirez8707 commented Jun 1, 2022

thomas-robinson Jun 2, 2022

thomas-robinson Jun 2, 2022

thomas-robinson Jun 2, 2022

thomas-robinson Jun 2, 2022

thomas-robinson Jun 2, 2022

thomas-robinson Jun 2, 2022

thomas-robinson Jun 2, 2022

thomas-robinson Jun 2, 2022

thomas-robinson Jun 2, 2022

thomas-robinson Jun 2, 2022

uramirez8707 Jun 2, 2022 •

edited

Loading

thomas-robinson Jun 2, 2022

uramirez8707 Jun 2, 2022

uramirez8707 commented Jun 2, 2022

thomas-robinson Jun 3, 2022

diag_axis io #982

diag_axis io #982

Conversation

uramirez8707 commented Jun 1, 2022

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

uramirez8707 Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uramirez8707 commented Jun 2, 2022

Choose a reason for hiding this comment

uramirez8707 Jun 2, 2022 •

edited

Loading