Skip to content

Converter for TEC data from Madrigal Database - #1658

Merged
BenjaminRuston merged 25 commits into
developfrom
feature/madrigal_converter
Oct 23, 2025
Merged

BenjaminRuston merged 25 commits into
developfrom
feature/madrigal_converter

Conversation

@haydenlj

@haydenlj haydenlj commented May 16, 2025 •

Copy link
Copy Markdown
Contributor

Description

Add a converter for TEC data from the Madrigal database

Issue(s) addressed

Resolves #1657

Dependencies

none

Impact

none

Checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have run the unit tests before creating the PR

@jhaiduce jhaiduce left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I haven't been able to get this to run (not your fault, it depends on ioda and I haven't figured out how to build and install ioda). But I do have a few comments based on reading through the code.

  • Until one looks at the code in detail it isn't clear whether this is meant for processing vertical TEC or the slant/line-of-sight TEC files. Both are available from Madrigal and the files for each are structured differently, so I'd suggest you specify which you one this script is meant for in the filename, the description string passed to ArgumentParser, and perhaps one or two other places (e.g. comments and/or function names).
  • I noticed there's nothing in the CMakeLists.txt that references this file, but I also see that not all the existing scripts in the gnssro directory are referenced in CMakeLists.txt so maybe it's fine that this one isn't either.



def main(args):
RO_files = args.input

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do the letters RO refer to here? If it's meant to be radio occultation, that's something of a misnomer for ground-based GNSS data (usually occultation refers to data collected from a spacecraft). This applies to the variable name RO_files, the directory name gnssro, and the metadata field 'satelliteConstellationRO'.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The directory structure has been updated to better reflect the data type. There are also now separate converters for space-based and ground-based TEC files from Madrigal

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking at this again, I'd suggest also changing the variable name RO_files, though that's less consequential than the things you already changed.

# Get command line arguments
parser = argparse.ArgumentParser(
description=(
'Reads the GNSS TEC data from netCDF file'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe the description here should refer to HDF5 rather than NetCDF.

@fcvdb fcvdb added the OBS OBS processing, UFO label May 22, 2025
@rtodling

Copy link
Copy Markdown
Contributor

I will add to the comments made above (wrt to nothing in the CMake) ... it would be nice to add an sample dataset and place some c-test that illustrates the conversion of TEC.

@haydenlj
haydenlj requested a review from ncrossette August 29, 2025 15:11
@haydenlj
haydenlj marked this pull request as ready for review October 13, 2025 23:51
@haydenlj
haydenlj requested a review from jhaiduce October 15, 2025 19:39
@haydenlj haydenlj mentioned this pull request Oct 16, 2025
1 of 4 tasks
@BenjaminRuston BenjaminRuston added the needs review Asking others to review - often used for pull requests label Oct 17, 2025
@BenjaminRuston

Copy link
Copy Markdown
Collaborator

there's a failure in the CI:

/workdir/bundle/jedi_ci_resources/github_api/check_run.py:474: DeprecationWarning: Call to deprecated class AppAuthentication. (Use github.Auth.AppInstallationAuth instead)

would re-trigger

@fcvdb

fcvdb commented Oct 17, 2025

Copy link
Copy Markdown
Collaborator

Is there any input file that could be used to test the decoders?

The Python scripts are not installed (copied) in the build/bin directory during the build. Is this on purpose?

@haydenlj

haydenlj commented Oct 17, 2025 •

Copy link
Copy Markdown
Contributor Author

Is there any input file that could be used to test the decoders?

The Python scripts are not installed (copied) in the build/bin directory during the build. Is this on purpose?

The files at /work2/noaa/jcsda/haydenlj/skylab/jedi-bundle_7-31-24/jedi-bundle/iodaconv/src/space_weather are the only ones we have downloaded. los_20220904.001.h5.hdf5 can be used for the line of sight converter and any of the gps*.hdf5 can be used for the gridded vTEC converter. jsn20240207j4.001.hdf5 can be used for the altimeter vTEC converter

@haydenlj

Copy link
Copy Markdown
Contributor Author

Is there any input file that could be used to test the decoders?

The Python scripts are not installed (copied) in the build/bin directory during the build. Is this on purpose?

Regarding the install, I don't know how to set that up. If that's something we need to do, I can find out how

@fcvdb

fcvdb commented Oct 21, 2025

Copy link
Copy Markdown
Collaborator

I modified the CMakeLists.txt to install the converters.

I tested with the following commands:

  python3 altimeter_spacebased_tec_madrigal2ioda.py -i jsn20240207j4.001.hdf5 -o output_altimeter.nc
  python3 gnss_gridded_ground_tec_madrigal2ioda.py  -i gps201022g.002.hdf5 -o output_gridded

In batch mode (needed memory):
python3 gnss_los_ground_tec_madrigal2ioda.py -i los_20220904.001.h5.hdf5 -o output_los

@fcvdb fcvdb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks Lindsey!

@jhaiduce jhaiduce left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks Lindsey!

@BenjaminRuston BenjaminRuston added ready for merge PR is reviewed and is ready for merge and removed needs review Asking others to review - often used for pull requests labels Oct 23, 2025
@BenjaminRuston
BenjaminRuston merged commit 8126202 into develop Oct 23, 2025
3 checks passed
@BenjaminRuston
BenjaminRuston deleted the feature/madrigal_converter branch October 23, 2025 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OBS OBS processing, UFO ready for merge PR is reviewed and is ready for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Converter for GNSS TEC from Madrigal database

6 participants