hdmf icon indicating copy to clipboard operation
hdmf copied to clipboard

Have VectorData expandable by default

Open mavaylon1 opened this issue 1 year ago • 2 comments

Motivation

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

Have VectorData expandable by default:

  • [ ] Check that reading existing non-expandable data into the now expandable VectorData is not a problem (due to shape).
  • [ ] Make to test that the data is wrapped with H5DataIO on read
  • [ ] Update VectorData
  • [ ] Add tests
  • [ ] Investigate VectorIndex. When indexing vector index that has a wrapped target vector data, an error arises.
  • [ ] TBD

Questions to look into:

  • Do we have tests where we look at add_row when the data is wrapped with H5DataIO? ---> Yes
  • If a user wanted to make a EnumData column extendable, would they wrap both data and elements with H5DataIO?

Ideas:

  • We support Zarr, so wrapping with H5DataIO for every instance does not work. Do we instead want a field that is default to True that will wrap data if true. In hdmf-zarr, we will override this to False (similar to how we import HERD in pynwb into a pynwb version in order to reset the typemap). We will also have logic that says if the user provides their own instance of DataIO, it will not wrap automatically either. We could also just skip the field and within HDMF-zarr just unwrap and reset data, but I understand if that is a little messy.

How to test the behavior?

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

Checklist

  • [ ] Did you update CHANGELOG.md with your changes?
  • [ ] Does the PR clearly describe the problem and the solution?
  • [ ] Have you reviewed our Contributing Guide?
  • [ ] Does the PR use "Fix #XXX" notation to tell GitHub to close the relevant issue numbered XXX when the PR is merged?

mavaylon1 avatar Mar 04 '24 17:03 mavaylon1

Note: This could be done by next release if backwards compatibility does not raise an issue. Otherwise it will be Future (the current set milestone).

mavaylon1 avatar Mar 04 '24 17:03 mavaylon1

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.53%. Comparing base (d85d0cb) to head (2f3a01a).

:exclamation: Current head 2f3a01a differs from pull request most recent head c0ce73b. Consider uploading reports for the commit c0ce73b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1064      +/-   ##
==========================================
- Coverage   88.68%   88.53%   -0.15%     
==========================================
  Files          45       45              
  Lines        9740     9606     -134     
  Branches     2768     2732      -36     
==========================================
- Hits         8638     8505     -133     
  Misses        778      778              
+ Partials      324      323       -1     

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

codecov[bot] avatar Mar 04 '24 17:03 codecov[bot]