NetCDF-4 support - #655
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds NetCDF CF‑1.8 FiberIO and helpers, updates plugin loading to skip broken/unavailable entry points with warnings, makes _get_prioritized_list ignore empty version maps, filters None-valued patch attributes before xarray conversion, and registers a NetCDF test asset and optional dependency. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #655 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 137 140 +3
Lines 12714 12888 +174
==========================================
+ Hits 12714 12888 +174
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cb836400e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fiberio = self._load_entry_point(name, loader) | ||
| if fiberio is not None: | ||
| self.register_fiberio(fiberio) |
There was a problem hiding this comment.
This code now tolerates a failed entry point load by returning None, but it leaves that format in known_formats; later _get_prioritized_list() iterates known_formats and assumes each format has at least one registered version, so a skipped plugin can trigger an IndexError on fiber_ios[0] and break unrelated format detection. This occurs whenever any plugin is present but unloadable (for example, missing optional dependencies), so the loader needs to exclude unregistered formats from prioritization or mark them as handled.
Useful? React with 👍 / 👎.
| if float(cf_version) >= 1.6: | ||
| return self.name, self.version |
There was a problem hiding this comment.
Using float(cf_version) misclassifies valid two-digit minor CF versions (for example, "1.10" becomes 1.1), so modern CF files can be incorrectly rejected by get_format() even though they should satisfy the >= 1.6 gate. This causes format auto-detection/read failures for legitimate NetCDF files with newer CF convention strings.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dascore/io/core.py`:
- Around line 408-418: The _load_entry_point function currently only catches
ImportError and AttributeError, so exceptions raised during plugin construction
(loader()()) will escape; update the except clause in _load_entry_point to catch
broad runtime exceptions from the loader/constructor (use except Exception as
exc to avoid catching BaseException like KeyboardInterrupt/SystemExit), emit the
same warnings.warn message including exc, and return None so load_plugins()
continues to isolate broken plugins.
In `@dascore/io/netcdf/core.py`:
- Around line 125-129: The CF version comparison using float(cf_version) is
incorrect for versions like "1.10"; update the check in the method using
cf_version (the block that currently does if float(cf_version) >= 1.6: return
self.name, self.version) to parse major and minor components instead (e.g.,
split on '.' and convert to ints) and compare tuples (major, minor) >= (1, 6);
handle missing minor component by treating it as 0 and keep the existing
ValueError/exception handling path for non-numeric strings.
In `@dascore/io/netcdf/utils.py`:
- Around line 483-490: The try/except at ref = ref_array[0] / dim_scale =
h5file[ref] block currently catches Exception broadly and swallows all errors;
narrow the except to only the expected exceptions (e.g., IndexError, KeyError,
TypeError) when resolving references for ref_array, h5file, dim_scale and
dim_name so genuine bugs are not hidden, and optionally log a debug message
before continuing; if any other unexpected exception occurs, allow it to
propagate (or re-raise) instead of silently passing.
In `@dascore/utils/io.py`:
- Around line 258-260: In patch_to_xarray() in dascore/utils/io.py, don't drop
None-valued attributes — replace the current filtered assignment of attrs (which
builds {key: value for key, value in dict(patch.attrs).items() if value is not
None}) with a direct shallow copy of the original attrs (e.g., attrs =
dict(patch.attrs)) so the in-memory converter is lossless; move any
NetCDF-specific sanitization (removing/transforming None values) into the NetCDF
write path (the function that writes patches to NetCDF) so round-trips patch ->
xarray -> patch preserve original attr keys and None values.
In `@tests/test_io/test_common_io.py`:
- Around line 57-61: The test gate function _has_xarray_netcdf_backend currently
returns True if a NetCDF backend is present but does not check for xarray,
causing NetCDFCFV18.read()/write() to raise when optional_import("xarray") is
missing; update _has_xarray_netcdf_backend to also require
importlib.util.find_spec("xarray") (i.e., return True only when xarray and at
least one of "netCDF4" or "h5netcdf" are importable) so tests will be skipped
when xarray is absent rather than failing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ae0e685-f1a6-4d2f-8018-3a64541fd719
📒 Files selected for processing (10)
dascore/data_registry.txtdascore/io/core.pydascore/io/netcdf/__init__.pydascore/io/netcdf/core.pydascore/io/netcdf/utils.pydascore/utils/io.pypyproject.tomltests/test_io/test_common_io.pytests/test_io/test_io_core.pytests/test_io/test_netcdf/test_netcdf.py
| def _load_entry_point(self, name: str, loader) -> FiberIO | None: | ||
| """Load one FiberIO entry point, skipping broken registrations.""" | ||
| try: | ||
| return loader()() | ||
| except (ImportError, AttributeError) as exc: | ||
| warnings.warn( | ||
| f"Skipping FiberIO plugin {name!r}: {exc}", | ||
| UserWarning, | ||
| stacklevel=2, | ||
| ) | ||
| return None |
There was a problem hiding this comment.
Also skip constructor-time plugin failures.
loader()() executes both the entry-point loader and the plugin constructor, but only ImportError/AttributeError are handled. A bad plugin with a required __init__ arg or any other constructor error will still abort load_plugins(), which defeats the new isolation behavior.
Suggested change
def _load_entry_point(self, name: str, loader) -> FiberIO | None:
"""Load one FiberIO entry point, skipping broken registrations."""
try:
return loader()()
- except (ImportError, AttributeError) as exc:
+ except Exception as exc:
warnings.warn(
f"Skipping FiberIO plugin {name!r}: {exc}",
UserWarning,
stacklevel=2,
)
return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dascore/io/core.py` around lines 408 - 418, The _load_entry_point function
currently only catches ImportError and AttributeError, so exceptions raised
during plugin construction (loader()()) will escape; update the except clause in
_load_entry_point to catch broad runtime exceptions from the loader/constructor
(use except Exception as exc to avoid catching BaseException like
KeyboardInterrupt/SystemExit), emit the same warnings.warn message including
exc, and return None so load_plugins() continues to isolate broken plugins.
| attrs = { | ||
| key: value for key, value in dict(patch.attrs).items() if value is not None | ||
| } |
There was a problem hiding this comment.
Don't make patch_to_xarray() lossy.
This is the generic in-memory converter, so dropping None-valued attrs here silently changes patch -> xarray -> patch round-trips. Please preserve the original attr dict in this helper and do the NetCDF-only sanitizing in the write path.
Suggested change
- attrs = {
- key: value for key, value in dict(patch.attrs).items() if value is not None
- }
+ attrs = dict(patch.attrs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| attrs = { | |
| key: value for key, value in dict(patch.attrs).items() if value is not None | |
| } | |
| attrs = dict(patch.attrs) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dascore/utils/io.py` around lines 258 - 260, In patch_to_xarray() in
dascore/utils/io.py, don't drop None-valued attributes — replace the current
filtered assignment of attrs (which builds {key: value for key, value in
dict(patch.attrs).items() if value is not None}) with a direct shallow copy of
the original attrs (e.g., attrs = dict(patch.attrs)) so the in-memory converter
is lossless; move any NetCDF-specific sanitization (removing/transforming None
values) into the NetCDF write path (the function that writes patches to NetCDF)
so round-trips patch -> xarray -> patch preserve original attr keys and None
values.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dascore/io/netcdf/core.py`:
- Around line 200-221: The writer currently only sets the "coordinates" list and
so auxiliary coordinate variables lose their dimension metadata used by
read_netcdf_coordinates(); update the write path after computing aux_coord_names
to also persist each aux coord variable's dimension metadata by setting
dataset[aux_name].attrs["_DASCORE_DIMS"] to the space-joined dimension names for
that coord (obtain dims from patch.coords.coord_map[name] or via the coord
object used by _iter_written_aux_coords); keep the existing
dataset["data"].attrs["coordinates"] assignment but add the loop that writes
_DASCORE_DIMS into each corresponding dataset variable so
scan()/read_netcdf_coordinates() can rebuild non-dimension coords.
In `@dascore/io/netcdf/utils.py`:
- Around line 480-490: When resolving DIMENSION_LIST for data_var, if any
reference lookup in the loop over dim_list (ref_array/ref/dim_scale/dim_name
access using h5file) raises IndexError/KeyError/TypeError you must treat the
whole DIMENSION_LIST resolution as failed: clear coord_order (e.g., set
coord_order = [] or equivalent) and break out so the later discovery fallback
will run; update the try/except around the loop that handles DIMENSION_LIST
resolution (the block referencing coord_order, DIMENSION_LIST, ref_array, ref,
dim_scale, dim_name, h5file) to reset coord_order and stop processing on the
first failure.
- Around line 101-108: The CF- token handling currently uses
conventions.split("CF-", 1)[1].split()[0].rstrip(",;") which fails when there is
no whitespace (e.g., "CF-1.8,ACDD-1.3"); update the logic in the CF branch (the
code that computes parts from the conventions variable, used by
_parse_cf_version/get_format) to extract only the version token by cutting the
substring after "CF-" and then splitting on common delimiters (comma, semicolon,
or whitespace) or by using a small regex to capture the version pattern (e.g.,
digits and dots) so examples like "CF-1.8", "CF-1.8,ACDD-1.3", and
"CF-1.8;something" all yield "1.8".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e51cc143-34ee-43bd-a4a5-c64ceeb84c50
📒 Files selected for processing (5)
dascore/io/core.pydascore/io/netcdf/core.pydascore/io/netcdf/utils.pytests/test_io/test_common_io.pytests/test_io/test_io_core.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_io/test_io_core.py
🚧 Files skipped from review as they are similar to previous changes (1)
- dascore/io/core.py
| for name, coord in patch.coords.coord_map.items(): | ||
| if coord._partial: | ||
| continue | ||
| data_array.coords[name].attrs.update(_coord_attrs(name, coord)) | ||
|
|
||
| global_attrs = get_cf_global_attrs(patch.attrs, self.version) | ||
| if patch.attrs.data_type: | ||
| global_attrs["source_data_type"] = patch.attrs.data_type | ||
| dataset = data_array.to_dataset() | ||
| dataset.attrs.update( | ||
| { | ||
| key: value | ||
| for key, value in global_attrs.items() | ||
| if value not in (None, "") | ||
| } | ||
| ) | ||
| dataset["data"].attrs.update( | ||
| get_cf_data_attrs(patch.attrs.data_type or "acoustic_signal") | ||
| ) | ||
| aux_coord_names = tuple(_iter_written_aux_coords(patch)) | ||
| if aux_coord_names: | ||
| dataset["data"].attrs["coordinates"] = " ".join(aux_coord_names) |
There was a problem hiding this comment.
Persist auxiliary coordinate dimension metadata on write.
read_netcdf_coordinates() only rebuilds non-dimension coordinates when the coord variable carries _DASCORE_DIMS (dascore/io/netcdf/utils.py, Lines 548-553), but this writer only records the coordinates list. Files written here will therefore lose auxiliary coords in scan(), even though read() can recover them via xarray.
Suggested fix
for name, coord in patch.coords.coord_map.items():
if coord._partial:
continue
- data_array.coords[name].attrs.update(_coord_attrs(name, coord))
+ coord_attrs = _coord_attrs(name, coord)
+ if name not in patch.dims:
+ coord_attrs["_DASCORE_DIMS"] = ",".join(patch.coords.dim_map[name])
+ data_array.coords[name].attrs.update(coord_attrs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for name, coord in patch.coords.coord_map.items(): | |
| if coord._partial: | |
| continue | |
| data_array.coords[name].attrs.update(_coord_attrs(name, coord)) | |
| global_attrs = get_cf_global_attrs(patch.attrs, self.version) | |
| if patch.attrs.data_type: | |
| global_attrs["source_data_type"] = patch.attrs.data_type | |
| dataset = data_array.to_dataset() | |
| dataset.attrs.update( | |
| { | |
| key: value | |
| for key, value in global_attrs.items() | |
| if value not in (None, "") | |
| } | |
| ) | |
| dataset["data"].attrs.update( | |
| get_cf_data_attrs(patch.attrs.data_type or "acoustic_signal") | |
| ) | |
| aux_coord_names = tuple(_iter_written_aux_coords(patch)) | |
| if aux_coord_names: | |
| dataset["data"].attrs["coordinates"] = " ".join(aux_coord_names) | |
| for name, coord in patch.coords.coord_map.items(): | |
| if coord._partial: | |
| continue | |
| coord_attrs = _coord_attrs(name, coord) | |
| if name not in patch.dims: | |
| coord_attrs["_DASCORE_DIMS"] = ",".join(patch.coords.dim_map[name]) | |
| data_array.coords[name].attrs.update(coord_attrs) | |
| global_attrs = get_cf_global_attrs(patch.attrs, self.version) | |
| if patch.attrs.data_type: | |
| global_attrs["source_data_type"] = patch.attrs.data_type | |
| dataset = data_array.to_dataset() | |
| dataset.attrs.update( | |
| { | |
| key: value | |
| for key, value in global_attrs.items() | |
| if value not in (None, "") | |
| } | |
| ) | |
| dataset["data"].attrs.update( | |
| get_cf_data_attrs(patch.attrs.data_type or "acoustic_signal") | |
| ) | |
| aux_coord_names = tuple(_iter_written_aux_coords(patch)) | |
| if aux_coord_names: | |
| dataset["data"].attrs["coordinates"] = " ".join(aux_coord_names) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dascore/io/netcdf/core.py` around lines 200 - 221, The writer currently only
sets the "coordinates" list and so auxiliary coordinate variables lose their
dimension metadata used by read_netcdf_coordinates(); update the write path
after computing aux_coord_names to also persist each aux coord variable's
dimension metadata by setting dataset[aux_name].attrs["_DASCORE_DIMS"] to the
space-joined dimension names for that coord (obtain dims from
patch.coords.coord_map[name] or via the coord object used by
_iter_written_aux_coords); keep the existing
dataset["data"].attrs["coordinates"] assignment but add the loop that writes
_DASCORE_DIMS into each corresponding dataset variable so
scan()/read_netcdf_coordinates() can rebuild non-dimension coords.
| # Handle various CF convention formats | ||
| if "CF-" in conventions: | ||
| # Format: "CF-1.8" or "CF-1.8, ACDD-1.3" | ||
| parts = conventions.split("CF-", 1)[1].split()[0].rstrip(",;") | ||
| return parts | ||
| elif conventions.startswith("CF "): | ||
| # Format: "CF 1.8" | ||
| return conventions.split()[1].rstrip(",;") |
There was a problem hiding this comment.
Don't require whitespace after the CF version token.
split()[0] only extracts the version when whitespace follows CF-.... For a common value like CF-1.8,ACDD-1.3, this returns 1.8,ACDD-1.3, so _parse_cf_version() later fails and get_format() rejects an otherwise supported file.
Suggested fix
- if "CF-" in conventions:
- # Format: "CF-1.8" or "CF-1.8, ACDD-1.3"
- parts = conventions.split("CF-", 1)[1].split()[0].rstrip(",;")
- return parts
+ if "CF-" in conventions:
+ # Format: "CF-1.8", "CF-1.8, ACDD-1.3", or "CF-1.8;ACDD-1.3"
+ suffix = conventions.split("CF-", 1)[1]
+ token = suffix.split(",", 1)[0].split(";", 1)[0].split()[0]
+ return token.rstrip(",;")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dascore/io/netcdf/utils.py` around lines 101 - 108, The CF- token handling
currently uses conventions.split("CF-", 1)[1].split()[0].rstrip(",;") which
fails when there is no whitespace (e.g., "CF-1.8,ACDD-1.3"); update the logic in
the CF branch (the code that computes parts from the conventions variable, used
by _parse_cf_version/get_format) to extract only the version token by cutting
the substring after "CF-" and then splitting on common delimiters (comma,
semicolon, or whitespace) or by using a small regex to capture the version
pattern (e.g., digits and dots) so examples like "CF-1.8", "CF-1.8,ACDD-1.3",
and "CF-1.8;something" all yield "1.8".
| if "DIMENSION_LIST" in data_var.attrs: | ||
| dim_list = data_var.attrs["DIMENSION_LIST"] | ||
| for ref_array in dim_list: | ||
| try: | ||
| ref = ref_array[0] | ||
| dim_scale = h5file[ref] | ||
| dim_name = dim_scale.name.strip("/") | ||
| coord_order.append(dim_name) | ||
| except (IndexError, KeyError, TypeError): | ||
| # If we can't resolve reference, we'll fall back to discovery | ||
| pass |
There was a problem hiding this comment.
Clear coord_order when DIMENSION_LIST resolution is incomplete.
If any reference lookup fails here, the code drops only that entry but keeps the partially resolved order. Because coord_order stays non-empty, the later “fall back to discovery” path never happens, and the remaining dimensions can be reordered incorrectly against the data array.
Suggested fix
if "DIMENSION_LIST" in data_var.attrs:
dim_list = data_var.attrs["DIMENSION_LIST"]
+ resolved_order = []
for ref_array in dim_list:
try:
ref = ref_array[0]
dim_scale = h5file[ref]
dim_name = dim_scale.name.strip("/")
- coord_order.append(dim_name)
+ resolved_order.append(dim_name)
except (IndexError, KeyError, TypeError):
- # If we can't resolve reference, we'll fall back to discovery
- pass
+ # Fall back to coordinate discovery if any dimension
+ # reference cannot be resolved.
+ resolved_order = []
+ break
+ coord_order = resolved_order📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if "DIMENSION_LIST" in data_var.attrs: | |
| dim_list = data_var.attrs["DIMENSION_LIST"] | |
| for ref_array in dim_list: | |
| try: | |
| ref = ref_array[0] | |
| dim_scale = h5file[ref] | |
| dim_name = dim_scale.name.strip("/") | |
| coord_order.append(dim_name) | |
| except (IndexError, KeyError, TypeError): | |
| # If we can't resolve reference, we'll fall back to discovery | |
| pass | |
| if "DIMENSION_LIST" in data_var.attrs: | |
| dim_list = data_var.attrs["DIMENSION_LIST"] | |
| resolved_order = [] | |
| for ref_array in dim_list: | |
| try: | |
| ref = ref_array[0] | |
| dim_scale = h5file[ref] | |
| dim_name = dim_scale.name.strip("/") | |
| resolved_order.append(dim_name) | |
| except (IndexError, KeyError, TypeError): | |
| # Fall back to coordinate discovery if any dimension | |
| # reference cannot be resolved. | |
| resolved_order = [] | |
| break | |
| coord_order = resolved_order |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dascore/io/netcdf/utils.py` around lines 480 - 490, When resolving
DIMENSION_LIST for data_var, if any reference lookup in the loop over dim_list
(ref_array/ref/dim_scale/dim_name access using h5file) raises
IndexError/KeyError/TypeError you must treat the whole DIMENSION_LIST resolution
as failed: clear coord_order (e.g., set coord_order = [] or equivalent) and
break out so the later discovery fallback will run; update the try/except around
the loop that handles DIMENSION_LIST resolution (the block referencing
coord_order, DIMENSION_LIST, ref_array, ref, dim_scale, dim_name, h5file) to
reset coord_order and stop processing on the first failure.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
dascore/utils/io.py (1)
258-262:⚠️ Potential issue | 🟠 MajorKeep
patch_to_xarray()lossless.Lines 258-262 drop
None-valued attrs in the generic patch→xarray converter, so non-NetCDF round-trips silently lose metadata too. Please keepattrs = dict(patch.attrs)here and do the NetCDF-only sanitizing inNetCDFCFV18.write()right before serialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/utils/io.py` around lines 258 - 262, The patch_to_xarray() converter currently filters out None-valued metadata which causes non-NetCDF round-trips to lose attrs; change the attrs construction back to a direct copy (attrs = dict(patch.attrs)) in patch_to_xarray(), and move the NetCDF-specific removal of None-valued attributes into NetCDFCFV18.write() so sanitization happens only immediately before NetCDF serialization (e.g., iterate over attrs in NetCDFCFV18.write() and drop None values there).dascore/io/netcdf/utils.py (1)
53-54:⚠️ Potential issue | 🟠 MajorHandle
CF-1.8,ACDD-1.3without requiring whitespace.Line 54 still uses
split()[0], so a commonConventionsvalue likeCF-1.8,ACDD-1.3yields1.8,ACDD-1.3and thenparse_cf_version()rejects an otherwise valid file. Split on comma/semicolon/whitespace before returning the token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/io/netcdf/utils.py` around lines 53 - 54, The current extraction of the CF version in utils.py (the branch that checks if "CF-" in conventions) uses conventions.split("CF-", 1)[1].split()[0]. Replace the final split()[0] with a split that delimits on commas, semicolons or whitespace (e.g., split on regex /[,;\s]/) so a value like "CF-1.8,ACDD-1.3" yields "1.8" and then parse_cf_version() will accept it; update the CF extraction code path that produces the token passed to parse_cf_version() accordingly.dascore/io/core.py (1)
412-417:⚠️ Potential issue | 🟠 MajorAlso isolate constructor-time plugin failures.
loader()()can fail during plugin construction, not just import. Catching onlyImportErrorandMissingOptionalDependencyErrorstill lets one bad entry point abortload_plugins(), which defeats the new skip-broken-plugin behavior.except Exception as excis the safer boundary here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/io/core.py` around lines 412 - 417, The current try/except around loader()() in dascore.io.core only catches ImportError and MissingOptionalDependencyError, so constructor-time errors in the plugin factory still bubble up and can abort load_plugins(); change the exception boundary to catch all exceptions (use except Exception as exc) around the loader()() call so that any error raised during import or plugin construction is caught, then warn with the same message handling (including the exc) and return None to preserve the skip-broken-plugin behavior; refer to the loader()() call and the surrounding load_plugins() logic and the MissingOptionalDependencyError symbol when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dascore/io/netcdf/core.py`:
- Around line 105-108: In scan(), stop materializing coord.values for every
coordinate; instead, when building coords from data_array.coords.items() only
include dimension coordinates (e.g., where coord.dims refers to a single
dimension and coord.ndim == 1) and record only metadata (dims, shape/length,
dtype or sizes) rather than reading the full array; skip auxiliary/2‑D coords
entirely in scan() and leave their full-value recovery to read(). Use the
existing symbols data_array.coords, coord.dims, coord.ndim,
coord.shape/coord.sizes and the scan() function to locate and change the logic
that currently accesses coord.values.
- Around line 48-49: The xarray calls currently use default engine resolution
which may fall back to scipy and break NetCDF-4 handling; update the three
usages of xr.open_dataset and Dataset.to_netcdf in dascore.io.netcdf.core.py to
explicitly request a NetCDF-4 capable engine by passing engine='netcdf4' if
available, else engine='h5netcdf' if available, and if neither backend is
installed raise a clear MissingOptionalDependency-like error explaining that
netCDF4 or h5netcdf is required for NETCDF_CF support; implement a small helper
(or reuse an existing optional-import helper) to detect availability of the
'netCDF4' and 'h5netcdf' packages, use that helper when opening datasets (the
xr.open_dataset call) and when writing (the to_netcdf calls in the
save/serialize method), and ensure the error message names the missing packages
and points to installation instructions.
---
Duplicate comments:
In `@dascore/io/core.py`:
- Around line 412-417: The current try/except around loader()() in
dascore.io.core only catches ImportError and MissingOptionalDependencyError, so
constructor-time errors in the plugin factory still bubble up and can abort
load_plugins(); change the exception boundary to catch all exceptions (use
except Exception as exc) around the loader()() call so that any error raised
during import or plugin construction is caught, then warn with the same message
handling (including the exc) and return None to preserve the skip-broken-plugin
behavior; refer to the loader()() call and the surrounding load_plugins() logic
and the MissingOptionalDependencyError symbol when making this change.
In `@dascore/io/netcdf/utils.py`:
- Around line 53-54: The current extraction of the CF version in utils.py (the
branch that checks if "CF-" in conventions) uses conventions.split("CF-",
1)[1].split()[0]. Replace the final split()[0] with a split that delimits on
commas, semicolons or whitespace (e.g., split on regex /[,;\s]/) so a value like
"CF-1.8,ACDD-1.3" yields "1.8" and then parse_cf_version() will accept it;
update the CF extraction code path that produces the token passed to
parse_cf_version() accordingly.
In `@dascore/utils/io.py`:
- Around line 258-262: The patch_to_xarray() converter currently filters out
None-valued metadata which causes non-NetCDF round-trips to lose attrs; change
the attrs construction back to a direct copy (attrs = dict(patch.attrs)) in
patch_to_xarray(), and move the NetCDF-specific removal of None-valued
attributes into NetCDFCFV18.write() so sanitization happens only immediately
before NetCDF serialization (e.g., iterate over attrs in NetCDFCFV18.write() and
drop None values there).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10e30aac-56c8-4e6e-a241-36a56063e603
📒 Files selected for processing (10)
dascore/data_registry.txtdascore/io/core.pydascore/io/netcdf/__init__.pydascore/io/netcdf/core.pydascore/io/netcdf/utils.pydascore/utils/io.pypyproject.tomltests/test_io/test_common_io.pytests/test_io/test_io_core.pytests/test_io/test_netcdf/test_netcdf.py
✅ Files skipped from review due to trivial changes (3)
- dascore/data_registry.txt
- tests/test_io/test_common_io.py
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- dascore/io/netcdf/init.py
| xr = optional_import("xarray") | ||
| with xr.open_dataset(resource) as dataset: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n dascore/io/netcdf/core.py | sed -n '45,105p'Repository: DASDAE/dascore
Length of output: 3181
🏁 Script executed:
# Check for optional dependencies and imports related to NetCDF backends
rg -A 3 "netcdf4|h5netcdf" dascore/io/netcdf/Repository: DASDAE/dascore
Length of output: 1045
🏁 Script executed:
# Check the beginning of the file for imports and class definition
cat -n dascore/io/netcdf/core.py | head -50Repository: DASDAE/dascore
Length of output: 2004
🏁 Script executed:
# Check setup.py or pyproject.toml for dependency declarations
fd -e "setup.py" -o -e "pyproject.toml" -o -e "setup.cfg" | head -5Repository: DASDAE/dascore
Length of output: 174
🏁 Script executed:
# Find setup files at repository root
find . -maxdepth 2 -type f \( -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" \) | head -10Repository: DASDAE/dascore
Length of output: 74
🏁 Script executed:
# Also check the optional_import function to see how it handles dependencies
rg -B 2 -A 10 "def optional_import" dascore/Repository: DASDAE/dascore
Length of output: 643
🏁 Script executed:
cat pyproject.tomlRepository: DASDAE/dascore
Length of output: 6079
🏁 Script executed:
# Get more context on optional_import function
rg -B 2 -A 20 "def optional_import" dascore/utils/misc.pyRepository: DASDAE/dascore
Length of output: 676
🏁 Script executed:
# Check xarray documentation on default engine behavior
# Also look for any existing engine handling in the codebase
rg -B 3 -A 3 "engine=" dascore/io/netcdf/Repository: DASDAE/dascore
Length of output: 40
🏁 Script executed:
# Check if h5netcdf is mentioned anywhere as an alternative
rg "h5netcdf" dascore/Repository: DASDAE/dascore
Length of output: 40
🏁 Script executed:
# Check if there's any test coverage for the netcdf module to see if netCDF4 is expected
fd -e "test*.py" -o -e "*test.py" | xargs grep -l "netcdf\|NetCDF" | head -5Repository: DASDAE/dascore
Length of output: 174
🏁 Script executed:
# Let's check what xarray's default engine behavior is by checking xarray version requirement
rg "xarray" pyproject.tomlRepository: DASDAE/dascore
Length of output: 71
🏁 Script executed:
# Look at the full write method to see all engine-related calls
cat -n dascore/io/netcdf/core.py | sed -n '74,95p'Repository: DASDAE/dascore
Length of output: 1082
🏁 Script executed:
# Check the scan method more thoroughly
cat -n dascore/io/netcdf/core.py | sed -n '96,130p'Repository: DASDAE/dascore
Length of output: 1864
Specify the NetCDF-4 engine explicitly in all three methods.
These calls currently rely on xarray's default engine resolution. In environments where xarray and scipy are installed but netCDF4 is unavailable, xarray will fall back to the scipy engine, which cannot properly handle NetCDF-4/HDF5 files. This violates the NETCDF_CF format contract and can produce silent failures or corrupted output. Please select engine='netcdf4' or engine='h5netcdf' explicitly in open_dataset() and to_netcdf() calls, and raise a clear missing-optional-dependency error when neither backend is available.
Affects: lines 49, 100 (open_dataset), and 91-94 (to_netcdf)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dascore/io/netcdf/core.py` around lines 48 - 49, The xarray calls currently
use default engine resolution which may fall back to scipy and break NetCDF-4
handling; update the three usages of xr.open_dataset and Dataset.to_netcdf in
dascore.io.netcdf.core.py to explicitly request a NetCDF-4 capable engine by
passing engine='netcdf4' if available, else engine='h5netcdf' if available, and
if neither backend is installed raise a clear MissingOptionalDependency-like
error explaining that netCDF4 or h5netcdf is required for NETCDF_CF support;
implement a small helper (or reuse an existing optional-import helper) to detect
availability of the 'netCDF4' and 'h5netcdf' packages, use that helper when
opening datasets (the xr.open_dataset call) and when writing (the to_netcdf
calls in the save/serialize method), and ensure the error message names the
missing packages and points to installation instructions.
| coords = { | ||
| name: (coord.dims, coord.values) | ||
| for name, coord in data_array.coords.items() | ||
| } |
There was a problem hiding this comment.
Don't materialize auxiliary coord arrays in scan().
coord.values here eagerly reads every coordinate variable, including 2-D/auxiliary coords, which can be nearly as large as the payload and defeats the metadata-only scan path. Restrict scan() to dimension-coordinate summaries and leave ancillary coordinate recovery to read().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dascore/io/netcdf/core.py` around lines 105 - 108, In scan(), stop
materializing coord.values for every coordinate; instead, when building coords
from data_array.coords.items() only include dimension coordinates (e.g., where
coord.dims refers to a single dimension and coord.ndim == 1) and record only
metadata (dims, shape/length, dtype or sizes) rather than reading the full
array; skip auxiliary/2‑D coords entirely in scan() and leave their full-value
recovery to read(). Use the existing symbols data_array.coords, coord.dims,
coord.ndim, coord.shape/coord.sizes and the scan() function to locate and change
the logic that currently accesses coord.values.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_io/test_netcdf/test_netcdf.py (3)
716-725: Validate compression metadata, not just data equality.This test currently checks round-trip values but does not assert that gzip + level were actually written. Add an HDF5 metadata assertion for the
datadataset compression settings.Example assertion extension
def test_compression_options(self, compressed_netcdf_file): """Test NetCDF file creation with compression options.""" path, original_patch = compressed_netcdf_file @@ np.testing.assert_array_almost_equal( original_patch.data, recovered_patch.data, decimal=6 ) + with h5py.File(path, "r") as h5file: + assert h5file["data"].compression == "gzip" + assert h5file["data"].compression_opts == 9🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/test_netcdf/test_netcdf.py` around lines 716 - 725, The test_compression_options currently only compares original_patch.data and recovered_patch.data; open the written NetCDF file (from compressed_netcdf_file -> path) with h5py and locate the 'data' dataset (or the exact dataset used by dc.read), then assert the dataset.attrs/compression properties indicate compression="gzip" and compression_opts equals the expected level (e.g., 4) — add these HDF5 metadata assertions after creating spool/recovered_patch in test_compression_options to validate the gzip + level were written.
20-20: Narrow thexarrayskip scope to xarray-dependent tests only.A module-level
importorskipcurrently skips utility tests that can run without xarray. Prefer per-test/class skips soget_cf_version/is_netcdf4_filecoverage still runs in minimal environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/test_netcdf/test_netcdf.py` at line 20, Remove the module-level pytest.importorskip("xarray") so non-xarray utility tests (e.g., get_cf_version and is_netcdf4_file) can run in minimal environments; instead, call pytest.importorskip("xarray") or use a skip marker (e.g., `@pytest.mark.skipif`(importlib.util.find_spec("xarray") is None, reason=...)) inside only the test functions or test classes that actually require xarray (identify those tests by references to xarray in their bodies), so xarray-dependent tests are skipped individually while other tests still execute.
589-601: Guardscipybackend usage and use context-managed reads.These tests assume
engine="scipy"is available and manually close handles. Add an explicit scipy skip and usewith xr.open_dataarray(...)to avoid handle leaks on failures.Proposed test hardening
def test_patch_to_xarray_can_be_serialized_with_xarray( self, patch_variant, tmp_path ): """A patch converted to xarray should round-trip through xarray IO.""" xr = pytest.importorskip("xarray") + pytest.importorskip("scipy") path = tmp_path / "xarray_roundtrip.nc" data_array = dc.io.patch_to_xarray(patch_variant) data_array.to_netcdf(path, engine="scipy") - reopened = xr.open_dataarray(path, engine="scipy") - round_tripped = dc.io.xarray_to_patch(reopened) - reopened.close() + with xr.open_dataarray(path, engine="scipy") as reopened: + round_tripped = dc.io.xarray_to_patch(reopened)def test_dascore_and_xarray_roundtrip_agree_for_non_dim_coords( self, patch_with_non_dim_coords, tmp_path ): """The same patch should round-trip through both IO paths identically.""" xr = pytest.importorskip("xarray") + pytest.importorskip("scipy") _require_xarray_netcdf_engine() @@ - reopened = xr.open_dataarray(path, engine="scipy") - xarray_patch = dc.io.xarray_to_patch(reopened) - reopened.close() + with xr.open_dataarray(path, engine="scipy") as reopened: + xarray_patch = dc.io.xarray_to_patch(reopened)Also applies to: 649-653
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/test_netcdf/test_netcdf.py` around lines 589 - 601, The test test_patch_to_xarray_can_be_serialized_with_xarray should explicitly guard use of the SciPy netCDF backend and avoid manual handle closing; call pytest.importorskip("scipy") before using engine="scipy" and replace xr.open_dataarray(path, engine="scipy") plus reopened.close() with a context-managed read using with xr.open_dataarray(path, engine="scipy") as reopened: then pass reopened to dc.io.xarray_to_patch; apply the same change to the sibling test that also uses engine="scipy" and manually closes the handle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_io/test_netcdf/test_netcdf.py`:
- Around line 773-775: The test currently picks fixed indices time_start =
time_coord[10] and time_end = time_coord[50], which is brittle; instead compute
indices from len(time_coord) (e.g., idx_start = len(time_coord)//4 and idx_end =
3*len(time_coord)//4) and set time_start = time_coord[idx_start] and time_end =
time_coord[idx_end] so the bounds adapt to the available time coordinate length
(refer to time_coord, time_start, time_end in the test).
---
Nitpick comments:
In `@tests/test_io/test_netcdf/test_netcdf.py`:
- Around line 716-725: The test_compression_options currently only compares
original_patch.data and recovered_patch.data; open the written NetCDF file (from
compressed_netcdf_file -> path) with h5py and locate the 'data' dataset (or the
exact dataset used by dc.read), then assert the dataset.attrs/compression
properties indicate compression="gzip" and compression_opts equals the expected
level (e.g., 4) — add these HDF5 metadata assertions after creating
spool/recovered_patch in test_compression_options to validate the gzip + level
were written.
- Line 20: Remove the module-level pytest.importorskip("xarray") so non-xarray
utility tests (e.g., get_cf_version and is_netcdf4_file) can run in minimal
environments; instead, call pytest.importorskip("xarray") or use a skip marker
(e.g., `@pytest.mark.skipif`(importlib.util.find_spec("xarray") is None,
reason=...)) inside only the test functions or test classes that actually
require xarray (identify those tests by references to xarray in their bodies),
so xarray-dependent tests are skipped individually while other tests still
execute.
- Around line 589-601: The test
test_patch_to_xarray_can_be_serialized_with_xarray should explicitly guard use
of the SciPy netCDF backend and avoid manual handle closing; call
pytest.importorskip("scipy") before using engine="scipy" and replace
xr.open_dataarray(path, engine="scipy") plus reopened.close() with a
context-managed read using with xr.open_dataarray(path, engine="scipy") as
reopened: then pass reopened to dc.io.xarray_to_patch; apply the same change to
the sibling test that also uses engine="scipy" and manually closes the handle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc30ca01-0618-4ed6-bdff-23a03271a8a1
📒 Files selected for processing (1)
tests/test_io/test_netcdf/test_netcdf.py
| time_start = time_coord[10] | ||
| time_end = time_coord[50] | ||
|
|
There was a problem hiding this comment.
Avoid fixed time indices in filtering test.
Hard-coded index selection makes this test fragile if example patch length changes. Derive bounds from len(time_coord) (e.g., quartiles) to keep it stable.
Suggested resilient indexing
- time_start = time_coord[10]
- time_end = time_coord[50]
+ n = len(time_coord)
+ time_start = time_coord[n // 4]
+ time_end = time_coord[(3 * n) // 4]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| time_start = time_coord[10] | |
| time_end = time_coord[50] | |
| n = len(time_coord) | |
| time_start = time_coord[n // 4] | |
| time_end = time_coord[(3 * n) // 4] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_io/test_netcdf/test_netcdf.py` around lines 773 - 775, The test
currently picks fixed indices time_start = time_coord[10] and time_end =
time_coord[50], which is brittle; instead compute indices from len(time_coord)
(e.g., idx_start = len(time_coord)//4 and idx_end = 3*len(time_coord)//4) and
set time_start = time_coord[idx_start] and time_end = time_coord[idx_end] so the
bounds adapt to the available time coordinate length (refer to time_coord,
time_start, time_end in the test).
1 participant
Summary
This PR adds a new NETCDF_CF FiberIO implementation for CF-style NetCDF-4 files.
The implementation uses:
It also registers the new IO via the dascore.fiber_io entry points and adds a large focused NetCDF test suite.
What Changed
Notes
Changelog
NETCDF_CFformat for reading and writing CF-convention NetCDF-4 files, detected with h5py and read and written through xarray.Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Bug Fixes
Plugin
Tests
Chores