Have VectorData expandable by default

[mavaylon1]

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]

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).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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     

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

[oruebel]

Hardcoding H5DataIO will probably create issues with ZarrIO. In the best case, ZarrIO ignores the wrapper and worst case it would fail.... I'm not sure which. For ZarrIO we should not need to wrap for datasets to be expandable, but the problem is, we won't know which backend will be used until we call write to know which wrapper to use.

Not sure what the best solution is for this. I could imagine a few approaches:

1. Have "expandable" as an option on the base DataIO class and wrap with DataIO as a generic wrapper that the specific backend (HD5FIO, ZarrIO etc.) would then need to know how to translate. ---> I think this should be feasible, but would require changes in both HDF5IO and ZarrIO
2. Maybe we would need to wrap in the build process ---> I think this could work, but may be tricky
3. Wrap with H5DataIO and require that the other backends know how to translate it ---> I don't like this one, because it hard-codes backend-specific wrappers in the Container

[mavaylon1]

This is what was going through my mind as well. I made a note documenting that. I had a expandable field idea.