Fix chunking bug with compound dtypes

[pauladkisson]

Fixes #1122

Comapare Dev Tests after the fix: https://github.com/catalystneuro/neuroconv/actions/runs/12960177532/job/36153641201 (2 failed -- see #1188)

To Dev Tests before: https://github.com/catalystneuro/neuroconv/actions/runs/12995279932/job/36241639533 (8 failed)

[pauladkisson]

@rly, lmk what you think

[pauladkisson]

@h-mayorquin, this is ready for review. Basically I use the hdmf.build.builders.BaseBuilder to check if a neurodata object would have a compound dtype. Most of the complexity is introduced by the need to find a match between the neurodata object and its location in the builder, which is outlined in the docstrings. Lmk what you think!

[h-mayorquin]

I did a first reading. Two things:

• I think, we should update hdmf on the pyproject to the latest version or greater.
• Looking at the tests that were failing before they seem related to the pixel mask. Can we create a more direct test of this in the dataset configuration tests? I think it would be better to have something more unit-test-like that would fail quicker if we break this (or if we can to refactor). Can we build something simpler with pixel-mask so we don't rely on the full segmentation conversion test?

[pauladkisson]

I think, we should update hdmf on the pyproject to the latest version or greater.

I updated to include everything <4, which zarr-related issues. Should be able to add hdmf 4.0 soon -- see: #1191

[pauladkisson]

Looking at the tests that were failing before they seem related to the pixel mask. Can we create a more direct test of this in the dataset configuration tests? I think it would be better to have something more unit-test-like that would fail quicker if we break this (or if we can to refactor). Can we build something simpler with pixel-mask so we don't rely on the full segmentation conversion test?

Definitely needs some unit tests. I'll put together some.

[pauladkisson]

From Meeting: move has_compound_dtype inside get_data_shape

[pauladkisson]

From Meeting: move has_compound_dtype inside get_data_shape

Actually, get_data_shape comes from hdmf.utils, so no way to move that in this PR. Also, if I remember correctly, they use get_data_shape in ways that would make incorporating this compound_dtype fix difficult.

[pauladkisson]

@h-mayorquin, i added tests, so this should be good to go!

[h-mayorquin]

My apologies regarding the get_data_shape function.

I propose we create a new function called get_full_data_shape in src/neuroconv/tools/hdmf.py and consolidate all related functionality there. It seems that this should be something provided by HDMF, and isolating this complexity from the rest of the code would be ideal.

There is also a coupling issue with the calculation of the manager for the entire file. I would prefer not to require from_neurodata_object to accept an additional argument that is build related. However, calculating the manager for every dataset is probably too expensive, and using the manager to build only for the dataset doesn't work (based on my initial attempt).

Requests:

1. Centralize Logic:
   Move the code to HDMF in the tools directory and create a new function, get_full_shape, that takes the builder as an optional argument so that all the logic is centralized. In the docstring, document that get_data_shape fails for compound objects and that this behavior is desired for building a dataset I/O configuration object.

2. Add Tests:
   Add a test that asserts the correct behavior of either the new function get_dataset_full_shape or the dataset IO configuration produced by the from_neurodata_object method. See the more concrete request on the review.

[h-mayorquin]

LGTM

Should we, in another PR, increase the floor of the hdmf version? Remove the ceiling?

[pauladkisson]

Should we, in another PR, increase the floor of the hdmf version? Remove the ceiling?

Sure, can you take care of that?

[h-mayorquin]

@pauladkisson
I can take care of the ceiling but I am not certain if we should increase the floor. Any idea?

[pauladkisson]

I can take care of the ceiling but I am not certain if we should increase the floor. Any idea?

I don't think we need to as long as it remains compatible with spikeinterface, etc.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.72%. Comparing base (89aac67) to head (a07135c).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1146      +/-   ##
==========================================
+ Coverage   89.65%   89.72%   +0.07%     
==========================================
  Files         129      129              
  Lines        8378     8420      +42     
==========================================
+ Hits         7511     7555      +44     
+ Misses        867      865       -2     

Flag	Coverage Δ
unittests	89.72% <100.00%> (+0.07%)	⬆️

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

Files with missing lines	Coverage Δ
src/neuroconv/tools/hdmf.py	97.65% <100.00%> (+1.02%)	⬆️
..._helpers/_configuration_models/_base_dataset_io.py	98.50% <100.00%> (+1.51%)	⬆️
...roconv/tools/nwb_helpers/_dataset_configuration.py	93.67% <100.00%> (+0.16%)	⬆️