Skip to content

Conversation

@amstokely
Copy link
Collaborator

@amstokely amstokely commented Jun 5, 2025

This PR updates the goes_abi converter in the obs2ioda library to improve compliance with the IODA v3 format and address several compilation issues.

Changes

  • Corrected dateTime representation:
    The dateTime variable is now written as an int64 array representing epoch seconds for each observation, instead of as a string. This aligns with the expected format for IODA v3 files.

  • Added dateTime units attribute:
    The NetCDF attribute for dateTime units was added ("seconds since 1970-01-01T00:00:00Z"), as required by the IODA v3 specification.

  • Compilation fixes:
    Resolved various minor compilation issues, including Fortran unit tests that previously failed to compile with the Intel Fortran compiler.

  • Simplified output file naming:
    The output file name now includes only the satellite ID and the date/time with minute precision. This reduces filename verbosity, making the files more straightforward to work with in scripts and data workflows.

  • Added a conda environment yaml for building the Python testing environment
    The conda environment file makes creating a conda environment for running the test suite seamlessly.

@amstokely amstokely requested review from ibanos90, jim-p-w and liujake June 5, 2025 17:16
@amstokely amstokely self-assigned this Jun 5, 2025
@jim-p-w
Copy link
Collaborator

jim-p-w commented Jun 5, 2025

It looks like this only addresses the goes_abi converter. What about the obs2ioda converter?
I see src/ncio_mod.f90 has this:
346: status = netcdfPutVar(netcdfID, ncname, str_ndatetime, "MetaData")

@amstokely
Copy link
Collaborator Author

It looks like this only addresses the goes_abi converter. What about the obs2ioda converter? I see src/ncio_mod.f90 has this: 346: status = netcdfPutVar(netcdfID, ncname, str_ndatetime, "MetaData")

While ncio_mod (and possibly gnssro_mod) define both datetime and dateTime variables, it's the dateTime variable that IODA recognizes and uses. IODA gives precedence to dateTime and ignores datetime if both are present. In a future PR, we should remove the unused datetime variable from ncio_mod, but for now it's harmless since it's ignored by IODA.

@jim-p-w
Copy link
Collaborator

jim-p-w commented Jun 5, 2025

It looks like this only addresses the goes_abi converter. What about the obs2ioda converter? I see src/ncio_mod.f90 has this: 346: status = netcdfPutVar(netcdfID, ncname, str_ndatetime, "MetaData")

While ncio_mod (and possibly gnssro_mod) define both datetime and dateTime variables, it's the dateTime variable that IODA recognizes and uses. IODA gives precedence to dateTime and ignores datetime if both are present. In a future PR, we should remove the unused datetime variable from ncio_mod, but for now it's harmless since it's ignored by IODA.

I don't see where dateTime is getting written in obs2ioda. Not being that familiar w/ the obs2ioda code,
I tried a case insensitive scan of the fortran code for 'cdfput.*datetime' and saw:

src/gnssro_mod.f90
460:      call check(netcdfPutAtt(ncid, 'min_datetime', gnssro_data%datetime(idx_min_time)))
461:      call check(netcdfPutAtt(ncid, 'max_datetime', gnssro_data%datetime(idx_max_time)))

src/ncio_mod.f90
150:      status = netcdfPutAtt(netcdfID, "min_datetime", xdata(ityp, itim)%min_datetime)
151:      status = netcdfPutAtt(netcdfID, "max_datetime", xdata(ityp, itim)%max_datetime)
346:               status = netcdfPutVar(netcdfID, ncname, str_ndatetime, "MetaData")

src/goes_abi_converter_mod.f90
147:        call check(netcdfPutVar(ncid, 'dateTime', datetime, 'MetaData'))

That isn't conclusive though. Can you point me to where obs2ioda writes dateTime?

@amstokely
Copy link
Collaborator Author

It looks like this only addresses the goes_abi converter. What about the obs2ioda converter? I see src/ncio_mod.f90 has this: 346: status = netcdfPutVar(netcdfID, ncname, str_ndatetime, "MetaData")

While ncio_mod (and possibly gnssro_mod) define both datetime and dateTime variables, it's the dateTime variable that IODA recognizes and uses. IODA gives precedence to dateTime and ignores datetime if both are present. In a future PR, we should remove the unused datetime variable from ncio_mod, but for now it's harmless since it's ignored by IODA.

I don't see where dateTime is getting written in obs2ioda. Not being that familiar w/ the obs2ioda code,

I tried a case insensitive scan of the fortran code for 'cdfput.*datetime' and saw:


src/gnssro_mod.f90

460:      call check(netcdfPutAtt(ncid, 'min_datetime', gnssro_data%datetime(idx_min_time)))

461:      call check(netcdfPutAtt(ncid, 'max_datetime', gnssro_data%datetime(idx_max_time)))



src/ncio_mod.f90

150:      status = netcdfPutAtt(netcdfID, "min_datetime", xdata(ityp, itim)%min_datetime)

151:      status = netcdfPutAtt(netcdfID, "max_datetime", xdata(ityp, itim)%max_datetime)

346:               status = netcdfPutVar(netcdfID, ncname, str_ndatetime, "MetaData")



src/goes_abi_converter_mod.f90

147:        call check(netcdfPutVar(ncid, 'dateTime', datetime, 'MetaData'))

That isn't conclusive though. Can you point me to where obs2ioda writes dateTime?

@jim-p-w dateTime is in the name_var_info array, which is defined in define_mod on line 127. ncio_mod uses this arrays to define variables. The validation test suite verifies that obs2ioda does write the dateTime variable for all file types in the MetaData group.

jim-p-w
jim-p-w previously approved these changes Jun 6, 2025
ibanos90
ibanos90 previously approved these changes Jun 12, 2025
Copy link
Collaborator

@ibanos90 ibanos90 left a comment

Choose a reason for hiding this comment

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

This branch successfully creates a GOES-ABI file that can be assimilated into our MPAS-JEDI applications. The output name also reflects better the extension and naming that follows closely other data types. Thanks for addressing all comments and fixing all issues, @amstokely!
For future reference, I tested compiling with GNU and INTEL and produces identical results with flags currently in use. Each file was tested within an MPAS-JEDI 3denvar run.

liujake
liujake previously approved these changes Jun 13, 2025
Copy link
Collaborator

@liujake liujake left a comment

Choose a reason for hiding this comment

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

There are some conflicts need to be resolved before merge.

@amstokely amstokely force-pushed the bug/dateTime branch 2 times, most recently from 2ee6258 to 3fafb80 Compare June 13, 2025 19:07
This PR updates the write_iodav3_netcdf subroutine to write explicit global variables for each dimension in the output NetCDF file. This change ensures compatibility with the IODA v3 format, which requires dimension variables to be present at the global level.

Update README with GOES-ABI Converter Installation and Usage Instructions (#83)

Summary:
This PR updates the README.md to document how to install and use the GOES-ABI converter included in obs2ioda. It also improves the instructions for running the validation test suite.

Changes:

Added instructions for enabling the GOES-ABI converter during the CMake configuration step (-DBUILD_GOES_ABI_CONVERTER=ON).
Added a section explaining how to run the GOES-ABI converter from the command line.
Expanded the test instructions to show how to run specific test suites using pytest -m, including the goes_abi marker.

Changed name of datetime variable to dateTime and fixed bug that occurred when writing dateTime with superobs enabled in goes_abi.

Defined linker language for fortran test targets.

Changed netcdf4 to netCDF4 in README instructions.

Fixed how Channel variable is set in goes_abi converter. Also added a function for creating fortran ctest tests.

Added units attribute for dateTime variable.

Updated test reference files.

Updated goes_abi ouput file naming.

Added docstring for  set_goes_abi_out_fname subroutine.

Added a conda env recipe yaml for creating the obs2ioda python testing env.

Also updated the README.

Created csh derecho environment scripts along with a bash intel derecho env script.
@amstokely
Copy link
Collaborator Author

There are some conflicts need to be resolved before merge.

@liujake I just rebased off main and all conflicts are resolved.

liujake
liujake previously approved these changes Jun 13, 2025
Copy link
Collaborator

@liujake liujake left a comment

Choose a reason for hiding this comment

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

Looks like conflicts are resolved. Better merge this today with another approval.

@amstokely amstokely changed the title Update datetime handling to use dateTime with correct type and fill behavior Update datetime handling to use dateTime with correct type and fill behavior and simplify environment setup Jun 13, 2025
@liujake
Copy link
Collaborator

liujake commented Jun 13, 2025

Can someone having admin permission of this repo remove the restriction for two approvals before merging? I think it is unnecessary to have this restriction. One approval is Ok to me.

@amstokely amstokely requested a review from liujake June 13, 2025 19:19
@ibanos90
Copy link
Collaborator

Can someone having admin permission of this repo remove the restriction for two approvals before merging? I think it is unnecessary to have this restriction. One approval is Ok to me.

Good point! I just changed it to 1

@amstokely amstokely merged commit 3c43d87 into main Jun 13, 2025
@ibanos90 ibanos90 deleted the bug/dateTime branch June 13, 2025 19:24
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