Skip to content

Commit 2e9565c

Browse files
bendichterclauderly
authored andcommitted
Make expandable a list; default expands only DynamicTable columns (#1439)
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: rly <310197+rly@users.noreply.github.com>
1 parent 4d11721 commit 2e9565c

4 files changed

Lines changed: 246 additions & 21 deletions

File tree

CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22

33
## Unreleased
44

5+
### Breaking changes
6+
- `HDF5IO` `expandable` argument is now a list of data type names instead of a boolean. The default is `("VectorData", "ElementIdentifiers")`, so only `DynamicTable` columns and id are expandable out of the box — previously every dataset with a matching spec shape was expanded. Datasets of types outside this list that previously were expandable by default will now default to fixed-shape on-disk layout; add the relevant type to `expandable` to restore prior behavior. Replace `expandable=True` with an explicit list (e.g. `["VectorData", "ElementIdentifiers", "MyType"]`) and `expandable=False` with `[]`; passing `True`/`False` now raises a `TypeError`. @bendichter @rly [#1439](https://github.com/hdmf-dev/hdmf/pull/1439)
7+
58
### Fixed
69
- Added missing validation for dataset reference target types to ensure correct `RefSpec.target_type` matching. @sejalpunwatkar [#1429](https://github.com/hdmf-dev/hdmf/pull/1429)
710

src/hdmf/backends/hdf5/h5tools.py

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -310,9 +310,13 @@ def __get_namespaces(cls, file_obj):
310310
{'name': 'herd', 'type': 'hdmf.common.resources.HERD',
311311
'doc': 'A HERD object to populate with references.',
312312
'default': None},
313-
{'name': 'expandable', 'type': bool, 'default': True,
314-
'doc': ('If True (default), datasets will be created as expandable by setting the maxshape '
315-
'based on the matching shape defined in the spec.')})
313+
{'name': 'expandable', 'type': (list, tuple),
314+
'default': ("VectorData", "ElementIdentifiers"),
315+
'doc': ('A list of data type names whose datasets (and subclasses) will be created as '
316+
'expandable — maxshape is set based on the matching shape defined in the spec. '
317+
'Default is ("VectorData", "ElementIdentifiers"), so only DynamicTable columns '
318+
'and id are expandable. Pass an empty list/tuple to disable automatic expansion '
319+
'entirely.')})
316320
def write(self, **kwargs):
317321
"""Write the container to an HDF5 file."""
318322
if self.__mode == 'r':
@@ -759,9 +763,13 @@ def close_linked_files(self):
759763
'default': True},
760764
{'name': 'export_source', 'type': str,
761765
'doc': 'The source of the builders when exporting', 'default': None},
762-
{'name': 'expandable', 'type': bool, 'default': True,
763-
'doc': ('If True (default), datasets will be created as expandable by setting the maxshape '
764-
'based on the matching shape defined in the spec.')})
766+
{'name': 'expandable', 'type': (list, tuple),
767+
'default': ("VectorData", "ElementIdentifiers"),
768+
'doc': ('A list of data type names whose datasets (and subclasses) will be created as '
769+
'expandable — maxshape is set based on the matching shape defined in the spec. '
770+
'Default is ("VectorData", "ElementIdentifiers"), so only DynamicTable columns '
771+
'and id are expandable. Pass an empty list/tuple to disable automatic expansion '
772+
'entirely.')})
765773
def write_builder(self, **kwargs):
766774
f_builder = popargs('builder', kwargs)
767775
self.logger.debug("Writing GroupBuilder '%s' to path '%s' with kwargs=%s"
@@ -939,9 +947,13 @@ def _filler():
939947
'default': True},
940948
{'name': 'export_source', 'type': str,
941949
'doc': 'The source of the builders when exporting', 'default': None},
942-
{'name': 'expandable', 'type': bool, 'default': True,
943-
'doc': ('If True (default), datasets will be created as expandable by setting the maxshape '
944-
'based on the matching shape defined in the spec.')},
950+
{'name': 'expandable', 'type': (list, tuple),
951+
'default': ("VectorData", "ElementIdentifiers"),
952+
'doc': ('A list of data type names whose datasets (and subclasses) will be created as '
953+
'expandable — maxshape is set based on the matching shape defined in the spec. '
954+
'Default is ("VectorData", "ElementIdentifiers"), so only DynamicTable columns '
955+
'and id are expandable. Pass an empty list/tuple to disable automatic expansion '
956+
'entirely.')},
945957
returns='the Group that was created', rtype=Group)
946958
def write_group(self, **kwargs):
947959
parent, builder = popargs('parent', 'builder', kwargs)
@@ -1042,9 +1054,13 @@ def write_link(self, **kwargs):
10421054
'default': True},
10431055
{'name': 'export_source', 'type': str,
10441056
'doc': 'The source of the builders when exporting', 'default': None},
1045-
{'name': 'expandable', 'type': bool, 'default': True,
1046-
'doc': ('If True (default), datasets will be created as expandable by setting the maxshape '
1047-
'based on the matching shape defined in the spec.')},
1057+
{'name': 'expandable', 'type': (list, tuple),
1058+
'default': ("VectorData", "ElementIdentifiers"),
1059+
'doc': ('A list of data type names whose datasets (and subclasses) will be created as '
1060+
'expandable — maxshape is set based on the matching shape defined in the spec. '
1061+
'Default is ("VectorData", "ElementIdentifiers"), so only DynamicTable columns '
1062+
'and id are expandable. Pass an empty list/tuple to disable automatic expansion '
1063+
'entirely.')},
10481064
returns='the Dataset that was created', rtype=Dataset)
10491065
def write_dataset(self, **kwargs): # noqa: C901
10501066
""" Write a dataset to HDF5
@@ -1071,12 +1087,17 @@ def write_dataset(self, **kwargs): # noqa: C901
10711087
else:
10721088
options['io_settings'] = {}
10731089

1074-
# Set maxshape to make a non-scalar dataset expandable but do not override existing settings
1090+
# Set maxshape to make datasets expandable. `expandable` is a list/tuple of data type names:
1091+
# a dataset is made expandable if its data type (or an ancestor) is in the list. The default
1092+
# list ("VectorData", "ElementIdentifiers") covers DynamicTable columns and id, which users
1093+
# commonly need to append to. An empty list disables automatic expansion.
10751094
if (
10761095
expandable
10771096
and 'maxshape' not in options['io_settings']
10781097
and np.ndim(data) != 0
10791098
and matched_spec_shape is not None
1099+
and self.manager.get_builder_dt(builder) is not None
1100+
and any(self.manager.is_sub_data_type(builder, dt) for dt in expandable)
10801101
):
10811102
options['io_settings']['maxshape'] = matched_spec_shape
10821103

tests/unit/common/test_table.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3211,6 +3211,7 @@ def test_append(self, cache_spec=False):
32113211

32123212
self.assertContainerEqual(read_container['table']['y'][-1], group1)
32133213

3214+
32143215
class TestVectorIndexDtype(TestCase):
32153216

32163217
def set_up_array_index(self):

tests/unit/test_io_hdf5_h5tools.py

Lines changed: 208 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,19 +23,22 @@
2323
from hdmf.build import GroupBuilder, DatasetBuilder, BuildManager, TypeMap, OrphanContainerBuildError, LinkBuilder
2424
from hdmf.container import Container, HERDManager
2525
from hdmf import Data, docval
26-
from hdmf.common import SimpleMultiContainer, get_hdf5io
26+
from hdmf.common import (DynamicTable, VectorData, SimpleMultiContainer, get_hdf5io, get_manager,
27+
get_type_map)
2728
from hdmf.data_utils import DataChunkIterator, GenericDataChunkIterator, InvalidDataIOError, append_data
29+
from hdmf.spec import DatasetSpec
2830
from hdmf.spec.catalog import SpecCatalog
2931
from hdmf.spec.namespace import NamespaceCatalog, SpecNamespace
3032
from hdmf.spec.spec import GroupSpec, DtypeSpec
31-
from hdmf.testing import TestCase, remove_test_file
33+
from hdmf.testing import TestCase, H5RoundTripMixin, remove_test_file
3234
from hdmf.common.resources import HERD
3335
from hdmf.term_set import TermSet, TermSetWrapper
3436
from hdmf.utils import get_data_shape
3537

3638
from tests.unit.helpers.utils import (Foo, FooBucket, FooFile, get_foo_buildmanager,
3739
Baz, BazData, BazCpdData, BazBucket, get_baz_buildmanager,
38-
CORE_NAMESPACE, get_temp_filepath, CacheSpecTestHelper,
40+
CORE_NAMESPACE, create_load_namespace_yaml, get_temp_filepath,
41+
CacheSpecTestHelper,
3942
CustomGroupSpec, CustomDatasetSpec, CustomSpecNamespace,
4043
QuxData, QuxBucket, get_qux_buildmanager)
4144
from tests.unit.helpers.io import DoNothingIO
@@ -878,14 +881,18 @@ def test_roundtrip_basic(self):
878881
read_foofile.buckets['bucket1'].foos['foo1'].my_data[:].tolist())
879882

880883
def test_roundtrip_basic_append(self):
881-
# Setup all the data we need
882-
foo1 = Foo('foo1', [1, 2, 3, 4, 5], "I am foo1", 17, 3.14)
884+
# Foo.my_data is an anonymous (untyped) dataset, so the default `expandable` list does
885+
# not cover it. Wrap its data in H5DataIO with an explicit maxshape to make it expandable.
886+
foo1 = Foo('foo1', H5DataIO(data=[1, 2, 3, 4, 5], maxshape=(None,)), "I am foo1", 17, 3.14)
883887
foobucket = FooBucket('bucket1', [foo1])
884888
foofile = FooFile(buckets=[foobucket])
885889

886890
with HDF5IO(self.path, manager=self.manager, mode='w') as io:
887891
io.write(foofile)
888892

893+
with h5py.File(self.path, 'r') as f:
894+
self.assertEqual(f['buckets/bucket1/foo_holder/foo1/my_data'].maxshape, (None,))
895+
889896
with HDF5IO(self.path, manager=self.manager, mode='a') as io:
890897
read_foofile = io.read()
891898
data = read_foofile.buckets['bucket1'].foos['foo1'].my_data
@@ -4175,7 +4182,7 @@ def test_expand_false(self):
41754182
foofile = FooFile(buckets=[foobucket])
41764183

41774184
with HDF5IO(self.path, manager=self.manager, mode='w') as io:
4178-
io.write(foofile, expandable=False)
4185+
io.write(foofile, expandable=[])
41794186

41804187
with HDF5IO(self.path, manager=self.manager, mode='r') as io:
41814188
read_foofile = io.read()
@@ -4191,7 +4198,7 @@ def test_multi_shape_no_labels(self):
41914198
manager = get_qux_buildmanager([[None, None], [None, 3]])
41924199

41934200
with HDF5IO(self.path, manager=manager, mode='w') as io:
4194-
io.write(quxbucket, expandable=True)
4201+
io.write(quxbucket, expandable=['QuxData'])
41954202

41964203
with HDF5IO(self.path, manager=manager, mode='r') as io:
41974204
read_quxbucket = io.read()
@@ -4204,7 +4211,7 @@ def test_expand_set_shape(self):
42044211
manager = get_qux_buildmanager([None, 3])
42054212

42064213
with HDF5IO(self.path, manager=manager, mode='w') as io:
4207-
io.write(quxbucket, expandable=True)
4214+
io.write(quxbucket, expandable=['QuxData'])
42084215

42094216
with HDF5IO(self.path, manager=manager, mode='r+') as io:
42104217
read_quxbucket = io.read()
@@ -4215,3 +4222,196 @@ def test_expand_set_shape(self):
42154222
[7, 8, 9]])
42164223
npt.assert_array_equal(read_quxbucket.qux_data.data[:], expected)
42174224
self.assertEqual(read_quxbucket.qux_data.data.maxshape, (None, 3))
4225+
4226+
4227+
class TestDefaultExpandable(H5RoundTripMixin, TestCase):
4228+
"""Test that VectorData, VectorIndex, and ElementIdentifiers datasets are expandable by default."""
4229+
4230+
def setUpContainer(self):
4231+
table = DynamicTable(name='table0', description='an example table')
4232+
table.add_column(name='foo', description='an int column')
4233+
table.add_column(name='bar', description='an indexed column', index=True)
4234+
table.add_row(foo=1, bar=[1, 2, 3])
4235+
table.add_row(foo=2, bar=[4, 5])
4236+
return table
4237+
4238+
def test_roundtrip(self):
4239+
super().test_roundtrip()
4240+
4241+
with h5py.File(self.filename, 'r') as f:
4242+
# VectorData columns should be expandable (maxshape has None in first dim)
4243+
self.assertEqual(f['foo'].maxshape, (None,))
4244+
self.assertIsNotNone(f['foo'].chunks)
4245+
4246+
# VectorIndex columns should be expandable
4247+
self.assertEqual(f['bar_index'].maxshape, (None,))
4248+
self.assertIsNotNone(f['bar_index'].chunks)
4249+
4250+
# Indexed VectorData should be expandable
4251+
self.assertEqual(f['bar'].maxshape, (None,))
4252+
self.assertIsNotNone(f['bar'].chunks)
4253+
4254+
# ElementIdentifiers (id column) should be expandable
4255+
self.assertEqual(f['id'].maxshape, (None,))
4256+
self.assertIsNotNone(f['id'].chunks)
4257+
4258+
4259+
class TestDefaultExpandableWithReferences(H5RoundTripMixin, TestCase):
4260+
"""Test that a VectorData column of references is expandable by default."""
4261+
4262+
def setUpContainer(self):
4263+
group1 = Container('group1')
4264+
group2 = Container('group2')
4265+
4266+
table = DynamicTable(name='table0', description='an example table')
4267+
table.add_column(name='ref', description='a reference column')
4268+
table.add_row(ref=group1)
4269+
table.add_row(ref=group2)
4270+
4271+
multi = SimpleMultiContainer(name='multi')
4272+
multi.add_container(group1)
4273+
multi.add_container(group2)
4274+
multi.add_container(table)
4275+
return multi
4276+
4277+
def test_roundtrip(self):
4278+
super().test_roundtrip()
4279+
4280+
with h5py.File(self.filename, 'r') as f:
4281+
ref_ds = f['table0/ref']
4282+
self.assertEqual(ref_ds.maxshape, (None,))
4283+
self.assertIsNotNone(ref_ds.chunks)
4284+
# Reference dtype should not bypass the expandable branch.
4285+
self.assertTrue(h5py.check_ref_dtype(ref_ds.dtype))
4286+
# Stored refs resolve to the expected target groups.
4287+
self.assertIsInstance(ref_ds[0], h5py.Reference)
4288+
self.assertEqual(f[ref_ds[0]].name, '/group1')
4289+
self.assertEqual(f[ref_ds[1]].name, '/group2')
4290+
4291+
4292+
class TestDefaultExpandableExplicitOverride(H5RoundTripMixin, TestCase):
4293+
"""Test that explicit H5DataIO settings are not overridden by the default expandable behavior."""
4294+
4295+
def setUpContainer(self):
4296+
# User explicitly sets maxshape — should be respected
4297+
foo = VectorData(
4298+
name='foo',
4299+
description='a column with explicit maxshape',
4300+
data=H5DataIO(data=[1, 2, 3], maxshape=(10,)),
4301+
)
4302+
table = DynamicTable(name='table0', description='an example table', columns=[foo])
4303+
return table
4304+
4305+
def test_roundtrip(self):
4306+
super().test_roundtrip()
4307+
4308+
with h5py.File(self.filename, 'r') as f:
4309+
# User's explicit maxshape should be preserved, not overridden
4310+
self.assertEqual(f['foo'].maxshape, (10,))
4311+
4312+
4313+
class TestExpandableArg(TestCase):
4314+
"""Test explicit values of the `expandable` argument to HDF5IO.write."""
4315+
4316+
def setUp(self):
4317+
self.filename = get_temp_filepath()
4318+
4319+
def tearDown(self):
4320+
remove_test_file(self.filename)
4321+
4322+
def _make_table(self):
4323+
table = DynamicTable(name='table0', description='an example table')
4324+
table.add_column(name='foo', description='an int column')
4325+
table.add_row(foo=1)
4326+
table.add_row(foo=2)
4327+
return table
4328+
4329+
def test_empty_disables_expansion(self):
4330+
with HDF5IO(self.filename, manager=get_manager(), mode='w') as io:
4331+
io.write(self._make_table(), expandable=[])
4332+
4333+
with h5py.File(self.filename, 'r') as f:
4334+
# With an empty expandable list, non-scalar datasets get a fixed shape, not (None,).
4335+
self.assertEqual(f['foo'].maxshape, (2,))
4336+
self.assertEqual(f['id'].maxshape, (2,))
4337+
4338+
def test_custom_list_expands_only_listed(self):
4339+
with HDF5IO(self.filename, manager=get_manager(), mode='w') as io:
4340+
io.write(self._make_table(), expandable=['VectorData'])
4341+
4342+
with h5py.File(self.filename, 'r') as f:
4343+
# VectorData column 'foo' is in the list → expandable.
4344+
self.assertEqual(f['foo'].maxshape, (None,))
4345+
# ElementIdentifiers 'id' is NOT in the list → fixed shape.
4346+
self.assertEqual(f['id'].maxshape, (2,))
4347+
4348+
def test_bool_raises_type_error(self):
4349+
"""Passing the old boolean API must fail loudly, per the 5.2.0 breaking change."""
4350+
with HDF5IO(self.filename, manager=get_manager(), mode='w') as io:
4351+
with self.assertRaises(TypeError):
4352+
io.write(self._make_table(), expandable=True)
4353+
with self.assertRaises(TypeError):
4354+
io.write(self._make_table(), expandable=False)
4355+
4356+
4357+
class TestDefaultExpandableSubclasses(TestCase):
4358+
"""Test that subclasses of VectorData and ElementIdentifiers are expandable by default."""
4359+
4360+
def setUp(self):
4361+
self.test_dir = tempfile.mkdtemp()
4362+
4363+
custom_ei_spec = DatasetSpec(
4364+
data_type_def='CustomElementIdentifiers',
4365+
data_type_inc='ElementIdentifiers',
4366+
doc='a custom ElementIdentifiers subclass',
4367+
)
4368+
custom_vd_spec = DatasetSpec(
4369+
data_type_def='CustomVectorData',
4370+
data_type_inc='VectorData',
4371+
doc='a custom VectorData subclass',
4372+
)
4373+
4374+
self.type_map = get_type_map()
4375+
create_load_namespace_yaml(
4376+
namespace_name=CORE_NAMESPACE,
4377+
specs=[custom_ei_spec, custom_vd_spec],
4378+
output_dir=self.test_dir,
4379+
incl_types={'hdmf-common': ['ElementIdentifiers', 'VectorData', 'DynamicTable']},
4380+
type_map=self.type_map,
4381+
)
4382+
self.manager = BuildManager(self.type_map)
4383+
4384+
self.CustomElementIdentifiers = self.type_map.get_dt_container_cls(
4385+
'CustomElementIdentifiers', CORE_NAMESPACE
4386+
)
4387+
self.CustomVectorData = self.type_map.get_dt_container_cls(
4388+
'CustomVectorData', CORE_NAMESPACE
4389+
)
4390+
4391+
self.filename = os.path.join(self.test_dir, 'test_expandable_subclasses.h5')
4392+
4393+
def tearDown(self):
4394+
shutil.rmtree(self.test_dir)
4395+
4396+
def test_subclasses_are_expandable(self):
4397+
custom_ids = self.CustomElementIdentifiers(name='id', data=[10, 20])
4398+
custom_col = self.CustomVectorData(
4399+
name='custom_col', description='a custom column', data=[1.0, 2.0]
4400+
)
4401+
table = DynamicTable(
4402+
name='table0',
4403+
description='an example table',
4404+
id=custom_ids,
4405+
columns=[custom_col],
4406+
)
4407+
4408+
with HDF5IO(self.filename, manager=self.manager, mode='w') as write_io:
4409+
write_io.write(table)
4410+
4411+
with h5py.File(self.filename, 'r') as f:
4412+
# ElementIdentifiers subclass (id) should be expandable
4413+
self.assertEqual(f['id'].maxshape, (None,))
4414+
self.assertIsNotNone(f['id'].chunks)
4415+
# VectorData subclass (custom_col) should be expandable
4416+
self.assertEqual(f['custom_col'].maxshape, (None,))
4417+
self.assertIsNotNone(f['custom_col'].chunks)

0 commit comments

Comments
 (0)