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

Add capability to read IAU increments in cubed sphere format #274

Closed
wants to merge 5 commits into from

Conversation

mark-a-potts
Copy link

This PR adds the capability to read in NetCDF-based cubed-sphere increments generated by JEDI. It includes a new (optional) namelist option to specify that non-gaussian formatted increment files are to be read.

Fixes # (issue)

How Has This Been Tested?

Two unit tests have been added to the code and can be run via ctest after the build.

Checklist:

Please check all whether they apply or not

  • 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

Copy link
Contributor

@lharris4 lharris4 left a comment

Choose a reason for hiding this comment

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

Thank you for designing this very useful functionality for the IAU on the native cubed-sphere grid, which promises to make a lot of the DA process smoother and can leverages some of JEDI's new features. There are some concerns I have about the coding that could be improved by leveraging existing functionality in FMS or by following similar codes already within FV3.

@@ -516,7 +516,7 @@ module fv_arrays_mod
!-----------------------------------------------------------------------------------------------

logical :: reset_eta = .false.
logical :: ignore_rst_cksum = .false. !< enfore (.false.) or override (.true.) data integrity restart checksums
Copy link
Contributor

Choose a reason for hiding this comment

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

I would beware changing this default unless absolutely necessary. This restart checksumming makes sure that the data isn't corrupted; it should be simple to set this namelist item when restarting from modified restart files.

tools/fv_iau_mod.F90 Show resolved Hide resolved
write(6,*) 'diff is ',abs(a - b),compvals

end function
logical function is_tracer(varname,tracer_idx)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be easier to use get_tracer_index() to see if a given field in the input data is defined as a tracer before reading it in, rather than using a list of hard-coded tracer names that would be difficult to extend to other model use cases that might have a different set of tracers or tracer names.

@@ -0,0 +1,551 @@
#define NC_ERR_STOP(status) \
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe most if not all of the I/O code in this file can be replaced with standard FMS calls. This could be done similar to that in get_nggps_ic which also reads in native-grid data.

Copy link

@DavidNew-NOAA DavidNew-NOAA Apr 17, 2024

Choose a reason for hiding this comment

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

@lharris4 If I recall correctly, FMS calls can only be used to read FMS-formatted restart files. Our aim with JEDI in the GDASApp and Global Workflow is to read increments in cubed-sphere history files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lucas is correct that FMS can read FMS and non-FMS created NetCDF files on the native grid or for any grid. Otherwise, our data_override capability to read in standardized forcing field datasets would not work.

For non-native grid NetCDF files and to keep each MPI-rank from reading in the full dataset, you can use:

  • use fms2_io::read_data to ingest the lat-lon data describing the fields in the file
  • determine indices that define a bounding box for the MPI-rank (with margins for interpolation)
  • use fms2_io::read_data to ingest only the portion of the field(s) within the bounding box indices
  • interpolate data to the native grid using a method defined in horiz_interp

For a native grid NetCDF defined on the cubed-sphere, one can use the fms2_io::read_data to read in the full field or if you know your particular indices you can read in a subset of the data. I believe you can easily use the restart functionality as well without any issues.

Choose a reason for hiding this comment

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

@bensonr Thanks for the info, I will look into changes using this functionality

@mark-a-potts
Copy link
Author

Thank you for designing this very useful functionality for the IAU on the native cubed-sphere grid, which promises to make a lot of the DA process smoother and can leverages some of JEDI's new features. There are some concerns I have about the coding that could be improved by leveraging existing functionality in FMS or by following similar codes already within FV3.

Thanks for take a look so quickly. I have been working with Cory Martin at EMC to test this out as part of a JEDI-based workflow, and it is still definitely in draft format. That said, this is exactly the kind of feedback we wanted to get before going too far down the rabbbit hole. I'll take a look at each of your suggestions and make the appropriate changes.

@lharris4
Copy link
Contributor

lharris4 commented Jun 7, 2023 via email

@lharris4
Copy link
Contributor

lharris4 commented Aug 7, 2023

Hi, @mark-a-potts . Is this still a WIP or is it ready for review?

@mark-a-potts
Copy link
Author

It is still in progress. I updated it recently so that EMC can do some testing with a current version of the UFS-WM. Thanks for checking!

@lharris4
Copy link
Contributor

lharris4 commented Aug 7, 2023 via email

@bensonr
Copy link
Contributor

bensonr commented Mar 8, 2024

@mark-a-potts - is this PR still being actively developed?

@mark-a-potts mark-a-potts marked this pull request as ready for review April 17, 2024 19:54
@mark-a-potts
Copy link
Author

I think EMC is about ready to start using this capability actively, so I just updated this again and have tested it with some accompanying changes to the fv3atm repo.

@DavidNew-NOAA
Copy link

DavidNew-NOAA commented May 17, 2024

Hi @laurenchilutti I'm working on an updated version of this PR that adds the ability to read cubed sphere increments both inside and outside the IAU framework. I'm hoping to submit a PR today or Monday once testing is complete.

@laurenchilutti
Copy link
Contributor

@DavidNew-NOAA Thank you for the update! When that new PR comes in would you like me to close this one?

@DavidNew-NOAA
Copy link

@laurenchilutti Yes, that's you could probably close it.

@DavidNew-NOAA
Copy link

@laurenchilutti #342 and FV3 PR #837 are ready for review. cc @XiaqiongZhou-NOAA @bensonr @lharris4 @mark-a-potts

@laurenchilutti
Copy link
Contributor

Closing, as PR #342 replaces this PR.

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.

5 participants