hdmf-dev / hdmf-zarr

Zarr I/O backend for HDMF
https://hdmf-zarr.readthedocs.io/
Other
7 stars 7 forks source link

Append References #203

Open mavaylon1 opened 3 weeks ago

mavaylon1 commented 3 weeks ago

Motivation

What was the reasoning behind this change? Please explain the changes briefly.

This is in conjunction with https://github.com/hdmf-dev/hdmf/pull/1135

We want to support appending a dataset of references.

How to test the behavior?

Show how to reproduce the new behavior (can be a bug fix or a new feature)

Checklist

mavaylon1 commented 3 weeks ago

@rly take a look. We need to merge the PR for zarr optional first, then the HDMF ticket related to this, then this. If you also agree with my approach to the paths (Even though it is a little hacky, I can't think of a better way given our conversation over slack), then I can wrap this up with tests tomorrow.

codecov-commenter commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 54.54545% with 10 lines in your changes missing coverage. Please review.

Project coverage is 83.73%. Comparing base (a508d58) to head (c54504b).

Files Patch % Lines
src/hdmf_zarr/zarr_utils.py 25.00% 6 Missing :warning:
src/hdmf_zarr/backend.py 71.42% 2 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #203 +/- ## ========================================== - Coverage 86.05% 83.73% -2.33% ========================================== Files 5 5 Lines 1162 1174 +12 Branches 287 289 +2 ========================================== - Hits 1000 983 -17 - Misses 107 134 +27 - Partials 55 57 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.11%. Comparing base (2ba6bc5) to head (798917f).

Files Patch % Lines
src/hdmf_zarr/backend.py 81.81% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #203 +/- ## ========================================== + Coverage 86.79% 87.11% +0.31% ========================================== Files 5 5 Lines 1159 1172 +13 Branches 285 286 +1 ========================================== + Hits 1006 1021 +15 + Misses 101 100 -1 + Partials 52 51 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mavaylon1 commented 2 weeks ago

Review Notes:

The ruff ignores are because changing it to "is" returns errors, i.e., the functionality changes for whatever reason. We can update this if we want, but not in this PR and it would be the lowest of priorities.

rly commented 4 days ago

Adding the new check raises this error when calling read_container.baz_data.data[10]:

if object_path is not None:
            try:
                target_zarr_obj = target_zarr_obj[object_path]
            except Exception:
>               raise ValueError("Found bad link to object %s in file %s" % (object_path, source_file))
E               ValueError: Found bad link to object /new in file /Users/rly/Documents/NWB/hdmf-zarr/test_io.zarr

src/hdmf_zarr/backend.py:741: ValueError

@mavaylon1 could you please look into this?

mavaylon1 commented 2 days ago

@rly This is done.

mavaylon1 commented 11 hours ago

@rly I made the changes. The main thing is to take a look at my notes. Your comment "the parent of the new child has not been built" is weird since the prior code said to build the parent. I assumed you meant the parent of the child could not be the root, leaving the path to the child being possibly incorrect if nested. To ensure a proper path to the child for the reference, we need to build the root. Let me know if that is what you meant.

I added a loop to do so. I did not add a nested test. If we really want one we can do so in a followup PR, but I think I want to explore the proxy idea in case that replaces this.