Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unstructured grid files axis #1114

Merged
merged 7 commits into from
Feb 1, 2023

Conversation

uramirez8707
Copy link
Contributor

Description
This is an update for the unstructured grid files.

Axis that are in the "unstructured" domain require a "compress" attribute for the combiner and PP. This attribute is passed in via a diag_axis_add_attribute call in the model code. The compress attribute indicates the names of the axis that were compressed for example grid_index:compress = "grid_yt grid_xt".

This PR:

  1. adds the metadata and the data for the axis structured axis in a unstructured grid file
  2. addresses a gnu compiler bug where the size of the axis_data was equal to 2 after it was passed in fms_diag_axis_init

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

Comment on lines 141 to 145
!TODO Passing in the axis_length because of a gnu issue where inside fms_diag_axis_init, the size of DATA
!was 2 which was causing the axis_data to not be written correctly...
diag_axis_init = fms_diag_object%fms_diag_axis_init(name, DATA, units, cart_name, long_name=long_name,&
& direction=direction, set_name=set_name, edges=edges, Domain=Domain, Domain2=Domain2, DomainU=DomainU, &
& aux=aux, req=req, tile_count=tile_count, domain_position=domain_position )
& aux=aux, req=req, tile_count=tile_count, domain_position=domain_position, axis_length=size(DATA(:)) )
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rem1776 @thomas-robinson FYI i saw this with gnu 9.3. Let me know if you have a better fix for this.

If I were to print the size(DATA(:)) here it gave me the correct answers, but inside fms_diag_axis_init it was equal to 2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this, and it sounded good when we discussed it.

Comment on lines 141 to 145
!TODO Passing in the axis_length because of a gnu issue where inside fms_diag_axis_init, the size of DATA
!was 2 which was causing the axis_data to not be written correctly...
diag_axis_init = fms_diag_object%fms_diag_axis_init(name, DATA, units, cart_name, long_name=long_name,&
& direction=direction, set_name=set_name, edges=edges, Domain=Domain, Domain2=Domain2, DomainU=DomainU, &
& aux=aux, req=req, tile_count=tile_count, domain_position=domain_position )
& aux=aux, req=req, tile_count=tile_count, domain_position=domain_position, axis_length=size(DATA(:)) )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this, and it sounded good when we discussed it.

Comment on lines +141 to +142
!TODO Passing in the axis_length because of a gnu issue where inside fms_diag_axis_init, the size of DATA
!was 2 which was causing the axis_data to not be written correctly...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a bug with a SPECIFIC gnu version, do we need to fix it or can we just say it's not compatible with that version? Is this a C5, CI, or dev box compiler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version of gnu is what we used in the CI
I can also reproduce in the dev box.

@@ -137,6 +140,8 @@ module fms_diag_axis_object_mod
TYPE(fmsDiagAttribute_type),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 :: structured_ids(2) !< If the axis is in the unstructured grid,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are the mappings from unstructured to structured, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are not mappings, they are the axis IDs of the original structured axis

diag_manager/fms_diag_axis_object.F90 Show resolved Hide resolved
this%axis_data = axis_data
this%length = axis_length
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is optional. What happens if it's not included?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to a required argument fms_diag_axis_init is only used locally here:

diag_axis_init = fms_diag_object%fms_diag_axis_init(name, DATA, units, cart_name, size(DATA(:)), &
& long_name=long_name, direction=direction, set_name=set_name, edges=edges, Domain=Domain, Domain2=Domain2, &
& DomainU=DomainU, aux=aux, req=req, tile_count=tile_count, domain_position=domain_position)

@@ -237,6 +243,7 @@ subroutine register_diag_axis_obj(this, axis_name, axis_data, units, cart_name,

this%nsubaxis = 0
this%num_attributes = 0
this%structured_ids = diag_null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be allocatable instead of used for every axis? Or should we make a new type that extends the fmsDiagFullAxis_type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to allocatables


select type (this)
type is (fmsDiagFullAxis_type)
this%structured_ids = axis_ids
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this%structured_ids should be allocated here and then set to axis_ids.

class(*), intent(in) :: compress_att(:) !< The compress attribute to parse
character(len=120) :: axis_names(2)

integer :: ios !< Errorcode after parsting the compress attribute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should say "after parsing"

Comment on lines 1080 to 1081
integer :: i,k !< For do loops
integer :: j !< diag_file%axis_ids(i) (for less typing)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confusing...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched this to a pointer

axis_ptr => diag_axis(diag_file%axis_ids(i))

@@ -368,15 +368,16 @@ end function fms_register_static_field
!> @brief Wrapper for the register_diag_axis subroutine. This is needed to keep the diag_axis_init
!! interface the same
!> @return Axis id
FUNCTION fms_diag_axis_init(this, axis_name, axis_data, units, cart_name, long_name, direction,&
& set_name, edges, Domain, Domain2, DomainU, aux, req, tile_count, domain_position, axis_length ) &
FUNCTION fms_diag_axis_init(this, axis_name, axis_data, units, cart_name, axis_length, long_name, direction,&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change the interface?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now a required arugment, so the interface changed.

@thomas-robinson thomas-robinson merged commit 34ef885 into NOAA-GFDL:dmUpdate Feb 1, 2023
rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024
* adds the compress attribute and axis data to the netcdf file

* Fix some typos, change axis_length to a required argument, change the structured_axis to allocatables
rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024
* adds the compress attribute and axis data to the netcdf file

* Fix some typos, change axis_length to a required argument, change the structured_axis to allocatables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants