Dedup logic

src/hdmf/backends/hdf5/h5tools.py

@@ -69,14 +69,16 @@
                 'default': None
             },
             {'name': 'herd_path', 'type': str,
-             'doc': 'The path to read/write the HERD file', 'default': None},)
+             'doc': 'The path to read/write the HERD file', 'default': None},
+            {'name': 'deduplicate_objects', 'type': bool,
+             'doc': 'whether to deduplicate identical container objects by creating soft links', 'default': True})
     def __init__(self, **kwargs):
         """Open an HDF5 file for IO.
         """
         self.logger = logging.getLogger('%s.%s' % (self.__class__.__module__, self.__class__.__qualname__))
-        path, manager, mode, comm, file_obj, driver, aws_region, herd_path = popargs('path', 'manager', 'mode',
+        path, manager, mode, comm, file_obj, driver, aws_region, herd_path, deduplicate_objects = popargs('path', 'manager', 'mode',
                                                                                        'comm', 'file', 'driver',
-                                                                                       'aws_region', 'herd_path',
+                                                                                       'aws_region', 'herd_path', 'deduplicate_objects',
                                                                                        kwargs)
 
         self.__open_links = []  # keep track of other files opened from links in this file
@@ -93,9 +95,9 @@
             raise UnsupportedOperation(msg)
 
         if manager is None:
-            manager = BuildManager(TypeMap(NamespaceCatalog()))
+            manager = BuildManager(TypeMap(NamespaceCatalog()), deduplicate_objects=deduplicate_objects)
         elif isinstance(manager, TypeMap):
-            manager = BuildManager(manager)
+            manager = BuildManager(manager, deduplicate_objects=deduplicate_objects)
         self.__driver = driver
         self.__aws_region = aws_region
         self.__comm = comm

src/hdmf/backends/io.py

@@ -22,9 +22,11 @@
             {"name": "source", "type": (str, Path),
              "doc": "the source of container being built i.e. file path", 'default': None},
             {'name': 'herd_path', 'type': str,
-             'doc': 'The path to read/write the HERD file', 'default': None},)
+             'doc': 'The path to read/write the HERD file', 'default': None},
+            {'name': 'deduplicate_objects', 'type': bool,
+             'doc': 'whether to deduplicate identical container objects by creating soft links', 'default': True})
     def __init__(self, **kwargs):
-        manager, source, herd_path = getargs('manager', 'source', 'herd_path', kwargs)
+        manager, source, herd_path, deduplicate_objects = getargs('manager', 'source', 'herd_path', 'deduplicate_objects', kwargs)
         if isinstance(source, Path):
             source = source.resolve()
         elif (isinstance(source, str) and
@@ -38,6 +40,7 @@
         self.__source = source
         self.herd_path = herd_path
         self.herd = None
+        self.__deduplicate_objects = deduplicate_objects
         self.open()
 
     @property

src/hdmf/build/manager.py

@@ -86,13 +86,19 @@ class BuildManager:
     A class for managing builds of AbstractContainers
     """
 
-    def __init__(self, type_map):
+    def __init__(self, type_map, deduplicate_objects=True):
+        """
+        Args:
+            type_map (TypeMap): the TypeMap to use for mapping container classes to specifications
+            deduplicate_objects (bool): whether to deduplicate identical container objects by creating soft links
+        """
         self.logger = logging.getLogger('%s.%s' % (self.__class__.__module__, self.__class__.__qualname__))
         self.__builders = dict()
         self.__containers = dict()
         self.__active_builders = set()
         self.__type_map = type_map
         self.__ref_queue = deque()  # a queue of the ReferenceBuilders that need to be added
+        self.__deduplicate_objects = deduplicate_objects
 
     @property
     def namespace_catalog(self):
@@ -102,6 +108,11 @@ def namespace_catalog(self):
     def type_map(self):
         return self.__type_map
 
+    @property
+    def deduplicate_objects(self):
+        """Whether to deduplicate identical container objects by creating soft links."""
+        return self.__deduplicate_objects
+
     @docval({"name": "object", "type": (BaseBuilder, AbstractContainer),
              "doc": "the container or builder to get a proxy for"},
             {"name": "source", "type": str,
@@ -260,8 +271,13 @@ def clear_cache(self):
 
     @docval({"name": "container", "type": AbstractContainer, "doc": "the container to get the builder for"})
     def get_builder(self, **kwargs):
-        """Return the prebuilt builder for the given container or None if it does not exist."""
+        """Return the prebuilt builder for the given container or None if it does not exist.
+
+        If deduplicate_objects is False, this will always return None to force creation of new builders.
+        """
         container = getargs('container', kwargs)
+        if not self.__deduplicate_objects:
+            return None
         container_id = self.__conthash__(container)
         result = self.__builders.get(container_id)
         return result

src/hdmf/build/objectmapper.py

@@ -281,6 +281,8 @@ def __check_edgecases(cls, spec, value, spec_dtype):  # noqa: C901
         """
         Check edge cases in converting data to a dtype
         """
+        # Check for complex numbers first
+        cls.__check_for_complex_numbers(value)
         if value is None:
             # Data is missing. Determine dtype from spec
             dt = spec_dtype

tests/unit/build_tests/test_complex_protection.py

@@ -0,0 +1,45 @@
+import unittest
+import numpy as np
+
+from hdmf.build.objectmapper import ObjectMapper
+from hdmf.spec import DatasetSpec
+from hdmf.testing import TestCase
+
+
+class TestComplexProtection(TestCase):
+    """Test that complex numbers are properly rejected."""
+
+    def setUp(self):
+        self.spec = DatasetSpec('an example dataset', 'float64', name='data')
+
+    def test_single_complex_number(self):
+        """Test that a single complex number is rejected."""
+        with self.assertRaises(ValueError) as cm:
+            ObjectMapper.convert_dtype(self.spec, 1 + 2j)
+        self.assertEqual(str(cm.exception), "Complex numbers are not supported")
+
+    def test_complex_array(self):
+        """Test that an array of complex numbers is rejected."""
+        with self.assertRaises(ValueError) as cm:
+            ObjectMapper.convert_dtype(self.spec, np.array([1 + 2j, 3 + 4j]))
+        self.assertEqual(str(cm.exception), "Complex numbers are not supported")
+
+    def test_complex_in_list(self):
+        """Test that a list containing complex numbers is rejected."""
+        with self.assertRaises(ValueError) as cm:
+            ObjectMapper.convert_dtype(self.spec, [1.0, 2 + 3j, 4.0])
+        self.assertEqual(str(cm.exception), "Complex numbers are not supported")
+
+    def test_real_array(self):
+        """Test that a real array is not rejected."""
+        ret, ret_dtype = ObjectMapper.convert_dtype(self.spec, np.array([1.0, 2.0, 3.0]))
+        self.assertIsInstance(ret, np.ndarray)
+
+    def test_real_number(self):
+        """Test that a real number is not rejected."""
+        ret, ret_dtype = ObjectMapper.convert_dtype(self.spec, 3.14)
+        self.assertIsInstance(ret, np.float64)
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/unit/build_tests/test_convert_dtype.py

@@ -569,3 +569,29 @@ def test_isodate_spec(self):
         self.assertEqual(ret, b'2020-11-10')
         self.assertIs(type(ret), bytes)
         self.assertEqual(ret_dtype, 'ascii')
+
+    def test_complex_number_rejection(self):
+        """Test that complex numbers are properly rejected."""
+        spec = DatasetSpec('an example dataset', 'float64', name='data')
+
+        # Test single complex number
+        with self.assertRaisesWith(ValueError, "Complex numbers are not supported"):
+            ObjectMapper.convert_dtype(spec, 1 + 2j)
+
+        # Test complex numpy array
+        with self.assertRaisesWith(ValueError, "Complex numbers are not supported"):
+            ObjectMapper.convert_dtype(spec, np.array([1 + 2j, 3 + 4j]))
+
+        # Test list containing complex numbers
+        with self.assertRaisesWith(ValueError, "Complex numbers are not supported"):
+            ObjectMapper.convert_dtype(spec, [1.0, 2 + 3j, 4.0])
+
+        # Test that real numbers still work
+        ret, ret_dtype = ObjectMapper.convert_dtype(spec, np.array([1.0, 2.0, 3.0]))
+        self.assertIsInstance(ret, np.ndarray)
+        self.assertEqual(ret_dtype, np.float64)
+
+        # Test that regular Python float still works
+        ret, ret_dtype = ObjectMapper.convert_dtype(spec, 3.14)
+        self.assertIsInstance(ret, np.float64)
+        self.assertEqual(ret_dtype, np.float64)

tests/unit/build_tests/test_deduplicate_objects.py

@@ -0,0 +1,148 @@
+"""Tests for the deduplicate_objects functionality in BuildManager"""
+
+from hdmf.build import GroupBuilder, DatasetBuilder, BuildManager, TypeMap, ObjectMapper
+from hdmf.spec import GroupSpec, AttributeSpec, DatasetSpec, SpecCatalog, SpecNamespace, NamespaceCatalog
+from hdmf.testing import TestCase
+from hdmf.container import Data
+
+from tests.unit.helpers.utils import Foo, CORE_NAMESPACE
+
+
+class FooMapper(ObjectMapper):
+    """Maps nested 'attr2' attribute on dataset 'my_data' to Foo.attr2 in constructor and attribute map"""
+
+    def __init__(self, spec):
+        super().__init__(spec)
+        my_data_spec = spec.get_dataset('my_data')
+        self.map_spec('attr2', my_data_spec.get_attribute('attr2'))
+
+
+class TestBuildManagerDeduplication(TestCase):
+    """Test BuildManager deduplication functionality"""
+
+    def setUp(self):
+        self.foo_spec = GroupSpec(
+            doc='A test group specification with a data type',
+            data_type_def='Foo',
+            datasets=[
+                DatasetSpec(
+                    doc='an example dataset',
+                    dtype='int',
+                    name='my_data',
+                    attributes=[
+                        AttributeSpec(
+                            name='attr2',
+                            doc='an example integer attribute',
+                            dtype='int'
+                        )
+                    ]
+                )
+            ],
+            attributes=[AttributeSpec('attr1', 'an example string attribute', 'text')]
+        )
+
+        self.spec_catalog = SpecCatalog()
+        self.spec_catalog.register_spec(self.foo_spec, 'test.yaml')
+        self.namespace = SpecNamespace(
+            'a test namespace',
+            CORE_NAMESPACE,
+            [{'source': 'test.yaml'}],
+            version='0.1.0',
+            catalog=self.spec_catalog)
+        self.namespace_catalog = NamespaceCatalog()
+        self.namespace_catalog.add_namespace(CORE_NAMESPACE, self.namespace)
+        self.type_map = TypeMap(self.namespace_catalog)
+        self.type_map.register_container_type(CORE_NAMESPACE, 'Foo', Foo)
+        self.type_map.register_map(Foo, FooMapper)
+
+    def test_default_deduplicate_objects_true(self):
+        """Test that deduplicate_objects defaults to True"""
+        manager = BuildManager(self.type_map)
+        self.assertTrue(manager.deduplicate_objects)
+
+    def test_deduplicate_objects_explicit_true(self):
+        """Test that deduplicate_objects can be explicitly set to True"""
+        manager = BuildManager(self.type_map, deduplicate_objects=True)
+        self.assertTrue(manager.deduplicate_objects)
+
+    def test_deduplicate_objects_explicit_false(self):
+        """Test that deduplicate_objects can be explicitly set to False"""
+        manager = BuildManager(self.type_map, deduplicate_objects=False)
+        self.assertFalse(manager.deduplicate_objects)
+
+    def test_get_builder_with_deduplication_enabled(self):
+        """Test that get_builder returns cached builder when deduplication is enabled"""
+        manager = BuildManager(self.type_map, deduplicate_objects=True)
+
+        # Create a simple Data container
+        container = Data(name="test_data", data=[1, 2, 3])
+
+        # Create and cache a builder
+        builder = DatasetBuilder(name="test_data", data=[1, 2, 3])
+        manager.prebuilt(container, builder)
+
+        # get_builder should return the cached builder
+        cached_builder = manager.get_builder(container)
+        self.assertIs(cached_builder, builder)
+
+    def test_get_builder_with_deduplication_disabled(self):
+        """Test that get_builder returns None when deduplication is disabled"""
+        manager = BuildManager(self.type_map, deduplicate_objects=False)
+
+        # Create a simple Data container
+        container = Data(name="test_data", data=[1, 2, 3])
+
+        # Create and cache a builder
+        builder = DatasetBuilder(name="test_data", data=[1, 2, 3])
+        manager.prebuilt(container, builder)
+
+        # get_builder should return None when deduplication is disabled
+        cached_builder = manager.get_builder(container)
+        self.assertIsNone(cached_builder)
+
+    def test_build_memoization_with_deduplication_enabled(self):
+        """Test that repeated builds return same builder when deduplication is enabled"""
+        manager = BuildManager(self.type_map, deduplicate_objects=True)
+
+        container_inst = Foo('my_foo', list(range(10)), 'value1', 10)
+
+        # Build twice - should get same builder
+        builder1 = manager.build(container_inst)
+        builder2 = manager.build(container_inst)
+
+        self.assertIs(builder1, builder2)
+
+    def test_build_no_memoization_with_deduplication_disabled(self):
+        """Test that repeated builds create new builders when deduplication is disabled"""
+        manager = BuildManager(self.type_map, deduplicate_objects=False)
+
+        container_inst = Foo('my_foo', list(range(10)), 'value1', 10)
+
+        # Build twice - should get different builders
+        builder1 = manager.build(container_inst)
+        builder2 = manager.build(container_inst)
+
+        self.assertIsNot(builder1, builder2)
+        # But they should have the same content
+        self.assertDictEqual(builder1, builder2)
+
+    def test_clear_cache_behavior(self):
+        """Test that clear_cache works regardless of deduplication setting"""
+        # Test with deduplication enabled
+        manager_true = BuildManager(self.type_map, deduplicate_objects=True)
+        container = Data(name="test_data", data=[1, 2, 3])
+        builder = DatasetBuilder(name="test_data", data=[1, 2, 3])
+        manager_true.prebuilt(container, builder)
+
+        self.assertIs(manager_true.get_builder(container), builder)
+        manager_true.clear_cache()
+        self.assertIsNone(manager_true.get_builder(container))
+
+        # Test with deduplication disabled
+        manager_false = BuildManager(self.type_map, deduplicate_objects=False)
+        manager_false.prebuilt(container, builder)
+
+        # Should return None even before clearing cache
+        self.assertIsNone(manager_false.get_builder(container))
+        manager_false.clear_cache()
+        self.assertIsNone(manager_false.get_builder(container))