DataWrangling cleanup: standardize download_dataset returns, drop ERA5 dead code#208
Open
DataWrangling cleanup: standardize download_dataset returns, drop ERA5 dead code#208
Conversation
- Drop the unused `ERA5_single_level_accumulated_variables` Set in `ERA5_single_levels.jl` — it was defined with a comment about CDS step types but never wired up to anything (no references in src/, ext/, test/, or examples/). - Drop the unused `Day` from the `using Dates: ...` import in `ERA5.jl` (only `DateTime`, `Month`, `Hour` are referenced). - Drop the redundant `metadata_path` from the `import` block — already brought in via `using NumericalEarth.DataWrangling: metadata_path`, and the ERA5 module never extends it (the `import` form is for extending only). - Delete the 1-arg `metadata_prefix(::ERA5Metadata)` / `(::ERA5PressureMetadata)` overloads. They duplicate the logic of the 4-arg form `metadata_prefix(dataset, name, date, region)` (which is what `metadata_filename` actually calls). Update the metadata_prefix unit tests to call the 4-arg form directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Make download_dataset consistently return paths instead of nothing:
String for single-file Metadatum, Vector{String} for multi-file Metadata.
ERA5 helpers now return their already-computed path lists rather than
re-deriving them via throwaway Metadatum construction.
Closes #201.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
download_era5_day and download_era5_multivar_day now return path vectors instead of nothing on the skip-existing short-circuit; update the assertions accordingly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The existing skip-existing tests only exercised single-calendar-day helper calls. Add tests that span two calendar days for the three parent dispatches (ERA5Metadata, ERA5PressureMetadata + names, and names + dataset + datetimes) so a regression that drops paths from one day group would fail loudly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
download_datasetreturn types across all ~17 methods:Stringfor single-fileMetadatum,Vector{String}for multi-fileMetadata. Drops the previous mix ofString/Vector{String}/nothingreturns. Closes Inconsistent return types acrossdownload_datasetimplementations #201.download_era5_day,download_era5_multivar_day) to return their already-computed path lists instead ofnothing, and to construct paths viajoinpath(dir, metadata_filename(...))directly rather than instantiating throwawayMetadatumobjects.ERA5_single_level_accumulated_variablesSet, unusedDayimport, redundantmetadata_pathimport, and 1-argmetadata_prefixoverloads that duplicated the 4-arg form (tests updated to call 4-arg form directly).Test plan
test_cds_downloading.jlpasses locally (skip-existing short-circuit assertions updated to expect path vectors)download_dataset(...)callers receive paths (previously discarded)🤖 Generated with Claude Code