Skip to content

Align HDF reader CF _FillValue with Zarr v3 semantics #420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Feb 28, 2025
Merged

Align HDF reader CF _FillValue with Zarr v3 semantics #420

merged 8 commits into from
Feb 28, 2025

Conversation

sharkinsspatial
Copy link
Collaborator

@codecov Codecov
Copy link

codecov bot commented Feb 4, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.64%. Comparing base (7cbc913) to head (5ddc85b).
Report is 21 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #420      +/-   ##
==========================================
- Coverage   88.74%   88.64%   -0.11%     
==========================================
  Files          28       28              
  Lines        1742     1752      +10     
==========================================
+ Hits         1546     1553       +7     
- Misses        196      199       +3
Files with missing lines Coverage Δ
virtualizarr/readers/hdf/hdf.py 95.31% <100.00%> (+0.23%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@abarciauskas-bgse abarciauskas-bgse mentioned this pull request Feb 15, 2025
Merged
12 tasks
@sharkinsspatial sharkinsspatial changed the title Remove unneccesary h5py fillvalue extraction logic. Align HDF reader CF _FillValue with Zarr v3 semantics Feb 20, 2025
@sharkinsspatial
Copy link
Collaborator Author

Closes #414
Closes #343

abarciauskas-bgse
abarciauskas-bgse reviewed Feb 20, 2025
View reviewed changes
virtualizarr/readers/hdf/hdf.py
An h5py group or dataset.
"""
fillvalue = None
for n, v in h5obj.attrs.items():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to integrate this so we don't have to loop through attrs twice? Just noticing we also loop through attrs in _extract_attrs

@sharkinsspatial sharkinsspatial marked this pull request as draft February 20, 2025 20:55
@sharkinsspatial sharkinsspatial marked this pull request as ready for review February 28, 2025 20:46
@sharkinsspatial sharkinsspatial removed the request for review from TomAugspurger February 28, 2025 20:52
TomNicholas
TomNicholas approved these changes Feb 28, 2025
View reviewed changes
Copy link
Member

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

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

Thanks @sharkinsspatial !

@TomNicholas TomNicholas merged commit c461b33 into main Feb 28, 2025
11 of 12 checks passed
@TomNicholas TomNicholas deleted the fix/fill_value branch February 28, 2025 22:12
@maxrjones maxrjones mentioned this pull request Feb 28, 2025
Closed
@TomNicholas TomNicholas mentioned this pull request Mar 2, 2025
Merged
TomNicholas added a commit that referenced this pull request Mar 2, 2025
@TomNicholas
Revert "Align HDF reader CF _FillValue with Zarr v3 semantics (#420)" (…
dbd3566 
…#457)

This reverts commit c461b33.
@maxrjones maxrjones restored the fix/fill_value branch March 2, 2025 22:23
chuckwondo added a commit to chuckwondo/VirtualiZarr that referenced this pull request Mar 3, 2025
@chuckwondo
Reapply "Align HDF reader CF _FillValue with Zarr v3 semantics (zarr-…
d903eea 
…developers#420)" (zarr-developers#457)

This reverts commit dbd3566.
@chuckwondo chuckwondo mentioned this pull request Mar 3, 2025
Merged
maxrjones pushed a commit that referenced this pull request Mar 3, 2025
@chuckwondo
Reapply HDF reader fillvalue (#460)
1cdc2f0 
* Reapply "Align HDF reader CF _FillValue with Zarr v3 semantics (#420)" (#457)

This reverts commit dbd3566.

* Reapply HDF fill value changes

Use context manager in tests to ensure resources are released.
This was referenced Mar 3, 2025
Closed
Closed
@maxrjones maxrjones deleted the fix/fill_value branch May 12, 2025 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abarciauskas-bgse abarciauskas-bgse abarciauskas-bgse left review comments

@TomNicholas TomNicholas TomNicholas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TypeError: ufunc 'isnan' not supported for the input types
3 participants
@sharkinsspatial @abarciauskas-bgse @TomNicholas