adds --riot-csv and --riot-positions to L2 command and riot_csv_writer#34
adds --riot-csv and --riot-positions to L2 command and riot_csv_writer#34s-pearce wants to merge 9 commits intoOSUGliders:mainfrom
Conversation
|
I haven't added any tests yet, but I wanted to get a functional review started so we could use this in next week's shakedown. |
There was a problem hiding this comment.
Pull request overview
Adds optional RIOT CSV export support to the L2 processing CLI, including an option to append/interpolate position fields, and introduces a dedicated writer module to generate RIOT $riotData-formatted CSV output.
Changes:
- Add
--riot-csvand--riot-positionsflags to theglide l2command. - Introduce
src/glide/riot_csv_writer.pyto extract RIOT variables from the L2 dataset and write RIOT-formatted CSV. - Update
.gitignoreto ignore PyCharm project metadata.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
src/glide/riot_csv_writer.py |
New RIOT CSV writing logic, including optional position interpolation/append. |
src/glide/cli.py |
Adds new l2 flags and calls the RIOT CSV writer after netCDF output is written. |
.gitignore |
Adds an ignore rule intended for PyCharm projects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _log.error("Dataset is missing required RIOT variables") | ||
| return |
There was a problem hiding this comment.
If the dataset is missing RIOT variables, the function logs an error and returns, but the l2 CLI command will still exit successfully even though the user explicitly requested --riot-csv. Consider raising an exception (or having the CLI convert this into a non-zero exit) so this failure mode is actionable and doesn't silently skip output generation.
| _log.error("Dataset is missing required RIOT variables") | |
| return | |
| missing_vars = set(riot_vars).difference(set(ds.data_vars)) | |
| _log.error( | |
| "Dataset is missing required RIOT variables: %s", | |
| ", ".join(sorted(missing_vars)), | |
| ) | |
| raise ValueError( | |
| "Dataset is missing required RIOT variables: " | |
| + ", ".join(sorted(missing_vars)) | |
| ) |
There was a problem hiding this comment.
@jessecusack , do you want this behavior, for the whole l2 command to fail if the RIOT variables are missing?
I guess it wouldn't matter if it did raise an exception because the netCDF file would have already been written at this point.
…instead of string
|
@s-pearce I think that we should include the bottle data in the level 2 output for debugging purposes. However, this will probably generate an error when generating the level 3 binned data. There isn't a robust mechanism for dropping variables during binning (it is hard coded). Binning the acoustic data is meaningless of course. I will create a separate issue to implement more robust variable dropping support. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Incorporated under #37 |
Adds 2 flag options to the
l2processing command.--riot-csv filenameoption which takes a filename/path string, and provided the riot variables are present in the TBD/EBD file provided to thel2command, will collect the data and write a $riotData formatted CSV file tofilename.--riot-positionsoption which if given with the--riot-csvflag, will append depth, lat, and lon position information to the RIOT data CSV file records. Depth, lat, and lon are interpolated to the timestamps that are part of the RIOT acoustic ping ranging data.Also adds the new file
riot_csv_writerwhich handles the logic of writing the CSV.Note: The RIOT variables are only available in the L2 data to write to the CSV if they are present in the configuration YAML file given to the
-coptions of thel2command. It is required to add them to the configuration file you are using.