Skip to content

Fix DASVader anonymous reference handling - #774

Merged
d-chambers merged 2 commits into
masterfrom
agent/fix-dasvader-anonymous-refs
Jul 24, 2026
Merged

Fix DASVader anonymous reference handling#774
d-chambers merged 2 commits into
masterfrom
agent/fix-dasvader-anonymous-refs

Conversation

@d-chambers

@d-chambers d-chambers commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Description

Legacy DASVader JLD2 files can contain readable HDF5 object references without linked object names. The compatibility guard introduced in #647 treated h5r.get_name(...) is None as proof that a reference could not be read, so DASCore raised DASVaderCompatibilityError on compatible h5py/HDF5 stacks before attempting dereferencing.

This change attempts h5[value] first and translates an actual KeyError into the existing compatibility error. It restores legacy-file reads with h5py 3.15/HDF5 1.14 while retaining the clear error under h5py 3.16/HDF5 2.0. The reader note now describes that behavior, and a self-contained regression test covers an anonymous but readable reference.

Validation

  • pytest -q tests/test_io/test_dasvader/test_dasvader.py with h5py 3.15.1/HDF5 1.14.6: 8 passed.
  • Same focused suite with h5py 3.16.0/HDF5 2.0.0: 7 passed, 1 compatibility skip.
  • Bounded read of the reported legacy file with h5py 3.15.1: one (235000, 2) float32 patch.
  • pytest -q tests: 6320 passed, 83 skipped, 4 xfailed.
  • pre-commit run --all: passed.

Checklist

I have (if applicable):

  • referenced the GitHub issue this PR closes.
  • documented the new feature with docstrings and/or appropriate doc page.
  • included tests. See testing guidelines.
  • added the "ready_for_review" tag once the PR is ready to be reviewed.

Summary by CodeRabbit

  • Bug Fixes
    • Improved compatibility when reading legacy DASVader files that include anonymous HDF5 references.
    • Reference resolution now uses direct dereferencing behavior when supported, with clearer fallback when resolution fails.
    • Compatibility guidance is provided when anonymous references cannot be dereferenced.
  • Tests
    • Added coverage for anonymous HDF5 reference dereferencing and confirmed expected behavior on dereference failure.

@d-chambers d-chambers added the ready_for_review PR is ready for review label Jul 24, 2026 — with ChatGPT Codex Connector
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: cca617c6-5621-4672-8a03-3e5b455e1024

📥 Commits

Reviewing files that changed from the base of the PR and between b1fc1a7 and 4659ee7.

📒 Files selected for processing (1)
  • tests/test_io/test_dasvader/test_dasvader.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/test_io/test_dasvader/test_dasvader.py

📝 Walkthrough

Walkthrough

Changes

DASVader reference handling now directly dereferences HDF5 references, raises compatibility errors when resolution fails, and tests both anonymous-reference success and unresolved-reference failure.

DASVader reference handling

Layer / File(s) Summary
Direct HDF5 reference resolution
dascore/io/dasvader/core.py, dascore/io/dasvader/utils.py
_dereference now indexes HDF5 references directly, converts failed dereferences into the existing compatibility error, and updates the related documentation.
Anonymous reference validation
tests/test_io/test_dasvader/test_dasvader.py
Adds coverage for resolving anonymous HDF5 dataset references and asserting compatibility errors for unresolved references.

Possibly related PRs

  • DASDAE/dascore#595: Initial DASVader format support introduced the related legacy reference handling.
  • DASDAE/dascore#601: Updates reference resolution in the same DASVader conversion path.
  • DASDAE/dascore#647: Includes related _dereference behavior and tests for legacy references.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: fixing DASVader anonymous reference handling.
Description check ✅ Passed The PR description includes the required Description and Checklist sections plus validation details, and is complete overall.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch agent/fix-dasvader-anonymous-refs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
tests/test_io/test_dasvader/test_dasvader.py (1)

243-255: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Cover the compatibility-error branch too.

This test only proves successful dereferencing. Add a focused case where h5[value] raises KeyError, then assert DASVaderCompatibilityError and its guidance; otherwise the central error-handling regression could break while this test remains green.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_io/test_dasvader/test_dasvader.py` around lines 243 - 255, Add a
focused test alongside test_dereference_anonymous_reference that exercises
_dereference when resolving the reference through h5[value] raises KeyError.
Assert that DASVaderCompatibilityError is raised and that its message includes
the expected compatibility guidance, while keeping the existing successful
dereference coverage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/test_io/test_dasvader/test_dasvader.py`:
- Around line 243-255: Add a focused test alongside
test_dereference_anonymous_reference that exercises _dereference when resolving
the reference through h5[value] raises KeyError. Assert that
DASVaderCompatibilityError is raised and that its message includes the expected
compatibility guidance, while keeping the existing successful dereference
coverage unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c5f6edc0-ab4a-432c-b297-2f9053c47f22

📥 Commits

Reviewing files that changed from the base of the PR and between 09aac2f and b1fc1a7.

📒 Files selected for processing (3)
  • dascore/io/dasvader/core.py
  • dascore/io/dasvader/utils.py
  • tests/test_io/test_dasvader/test_dasvader.py

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.93%. Comparing base (09aac2f) to head (4659ee7).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #774   +/-   ##
=======================================
  Coverage   99.93%   99.93%           
=======================================
  Files         145      145           
  Lines       12859    12859           
=======================================
  Hits        12851    12851           
  Misses          8        8           
Flag Coverage Δ
unittests 99.93% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

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

@d-chambers
d-chambers merged commit 2426bbc into master Jul 24, 2026
39 of 150 checks passed
@d-chambers
d-chambers deleted the agent/fix-dasvader-anonymous-refs branch July 24, 2026 17:01
@d-chambers d-chambers mentioned this pull request Aug 4, 2026
4 tasks
@d-chambers d-chambers removed the ready_for_review PR is ready for review label Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant