Fix HDFVirtualBackend handling of non coordinate dimension HDF datasets. #410

sharkinsspatial · 2025-01-31T17:04:33Z

Closes HDF reader creating spurious dimension coordinates #401
Tests added
Tests passing
Full type hint coverage

codecov · 2025-01-31T17:05:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.97%. Comparing base (81a76f0) to head (ce118fb).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #410      +/-   ##
==========================================
+ Coverage   75.61%   77.97%   +2.35%     
==========================================
  Files          33       31       -2     
  Lines        1993     1834     -159     
==========================================
- Hits         1507     1430      -77     
+ Misses        486      404      -82

Files with missing lines	Coverage Δ
virtualizarr/readers/hdf/hdf.py	`95.08% <100.00%> (+0.40%)`	⬆️

... and 3 files with indirect coverage changes

sharkinsspatial · 2025-01-31T17:13:09Z

For some reason I can't add @jsignell as a reviewer for this PR directly.

jsignell

Nice! I just took a little read through but it works great!

jsignell · 2025-01-31T18:16:26Z

virtualizarr/tests/test_xarray.py


    vds = open_virtual_dataset(simple_netcdf4, loadable_variables=["foo"])
-    assert vds.virtualize.nbytes == 104
+    assert vds.virtualize.nbytes == 48


jsignell · 2025-01-31T18:43:17Z

virtualizarr/readers/hdf/hdf.py

+        dimension_names = []
+        non_coordinate_dimesion_variables = []
+        for name, obj in group.items():
+            if "_Netcdf4Dimid" in obj.attrs:
+                dimension_names.append(name)
+            for name, obj in group.items():
+                if type(obj) is h5py.Dataset:
+                    if obj.id.get_storage_size() == 0 and name in dimension_names:
+                        non_coordinate_dimesion_variables.append(name)
+
+        return non_coordinate_dimesion_variables


Could this be simplified to:

Suggested change

dimension_names = []

non_coordinate_dimesion_variables = []

for name, obj in group.items():

if "_Netcdf4Dimid" in obj.attrs:

dimension_names.append(name)

for name, obj in group.items():

if type(obj) is h5py.Dataset:

if obj.id.get_storage_size() == 0 and name in dimension_names:

non_coordinate_dimesion_variables.append(name)

return non_coordinate_dimesion_variables

return [

name

for name, obj in group.items()

if isinstance(obj, h5py.Dataset)

and "_Netcdf4Dimid" in obj.attrs

and obj.id.get_storage_size() == 0

]

I think so, but Sean's version it's melting my brain to look at

I tried this swap locally and it worked fine.

This was actually an indent error on my part 🤦 . We need to first scan all items to determine all dimensions and then subsequently check if any of the those dimensions are represented by 0 size HDF5 datasets.

You are iterating over group.items both times though right? So you could collapse that into one iteration. There is no way to get a name in dimension_names without the related obj having obj.attrs == "_Netcdf4Dimid".

But this is all just a nitpick. I think the only part that matters is using isinstance rather than type

jsignell · 2025-01-31T18:44:35Z

virtualizarr/readers/hdf/hdf.py

+        non_coordinate_dimesion_vars = HDFVirtualBackend._find_non_coord_dimension_vars(
+            group=g
+        )
+        drop_variables = list(set(drop_variables + non_coordinate_dimesion_vars))


nitpick but I think this is more legible and faster:

Suggested change

drop_variables = list(set(drop_variables + non_coordinate_dimesion_vars))

drop_variables = [*drop_variables, *non_coordinate_dimension_vars]

Speed of this is not important, and I think the first one makes intent clearer (if the intent is to de-duplicate using set?)

Good point! I was getting too nitpicky. Just the typo then :)

jsignell · 2025-01-31T18:54:54Z

virtualizarr/readers/hdf/hdf.py

+        non_coordinate_dimesion_vars = HDFVirtualBackend._find_non_coord_dimension_vars(
+            group=g
+        )


nitpick: typo (dimesion -> dimension)

Suggested change

non_coordinate_dimesion_vars = HDFVirtualBackend._find_non_coord_dimension_vars(

group=g

)

non_coordinate_dimension_vars = (

HDFVirtualBackend._find_non_coord_dimension_vars(group=g)

)

TomNicholas

Looks good to me, thanks @sharkinsspatial !

TomNicholas · 2025-01-31T19:51:15Z

For some reason I can't add @jsignell as a reviewer for this PR directly.

Same reason as this #392 (comment), and I forgot to ask Josh to add you to the team that has merge permissions 😞

sharkinsspatial added 3 commits January 31, 2025 11:46

Do not create variables for non coordinate dimension hdf datasets.

7647a40

Revert test changes to avoid HDFVirtualBackend errors from #395.

1e916b5

Re-enable xfailed roundtrip integration test.

fceb871

sharkinsspatial requested a review from TomNicholas January 31, 2025 17:04

sharkinsspatial had a problem deploying to test-release January 31, 2025 17:05 — with GitHub Actions Failure

sharkinsspatial temporarily deployed to test-release January 31, 2025 17:08 — with GitHub Actions Inactive

Fix HDF5 type usage.

5d12003

sharkinsspatial force-pushed the fix/non_coord_dims branch from 4002179 to 5d12003 Compare January 31, 2025 17:10

sharkinsspatial temporarily deployed to test-release January 31, 2025 17:11 — with GitHub Actions Inactive

jsignell approved these changes Jan 31, 2025

View reviewed changes

jsignell mentioned this pull request Jan 31, 2025

Update to zarr 3 and main kerchunk #406

Merged

6 tasks

TomNicholas approved these changes Jan 31, 2025

View reviewed changes

Fix indent error for scanning HDF5 items.

ce118fb

sharkinsspatial temporarily deployed to test-release January 31, 2025 21:07 — with GitHub Actions Inactive

sharkinsspatial merged commit 61847e9 into main Jan 31, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HDFVirtualBackend handling of non coordinate dimension HDF datasets. #410

Fix HDFVirtualBackend handling of non coordinate dimension HDF datasets. #410

sharkinsspatial commented Jan 31, 2025

codecov bot commented Jan 31, 2025 •

edited

Loading

sharkinsspatial commented Jan 31, 2025

jsignell left a comment

jsignell Jan 31, 2025

jsignell Jan 31, 2025

TomNicholas Jan 31, 2025 •

edited

Loading

jsignell Jan 31, 2025

sharkinsspatial Jan 31, 2025

jsignell Jan 31, 2025

jsignell Jan 31, 2025

TomNicholas Jan 31, 2025

jsignell Jan 31, 2025

jsignell Jan 31, 2025

TomNicholas left a comment

TomNicholas commented Jan 31, 2025

	drop_variables = list(set(drop_variables + non_coordinate_dimesion_vars))
	drop_variables = [drop_variables, non_coordinate_dimension_vars]

-        non_coordinate_dimesion_vars = HDFVirtualBackend._find_non_coord_dimension_vars(
-            group=g
-        )
+        non_coordinate_dimension_vars = (
+            HDFVirtualBackend._find_non_coord_dimension_vars(group=g)
+        )

Fix HDFVirtualBackend handling of non coordinate dimension HDF datasets. #410

Fix HDFVirtualBackend handling of non coordinate dimension HDF datasets. #410

Conversation

sharkinsspatial commented Jan 31, 2025

codecov bot commented Jan 31, 2025 • edited Loading

Codecov Report

sharkinsspatial commented Jan 31, 2025

jsignell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomNicholas Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomNicholas left a comment

Choose a reason for hiding this comment

TomNicholas commented Jan 31, 2025

codecov bot commented Jan 31, 2025 •

edited

Loading

TomNicholas Jan 31, 2025 •

edited

Loading