From a046bd1bec513a81ada203ce3260b2293b8c3a80 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Tue, 18 Aug 2026 19:51:23 -0600 Subject: [PATCH 1/2] Performance improvement for dynamic table add_row --- CHANGELOG.md | 2 + src/hdmf/common/table.py | 30 +++++++++++++- tests/unit/common/test_table.py | 73 ++++++++++++++++++++++++++++++++- 3 files changed, 103 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bf465a4b7..909ed50a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,10 @@ - Added support for hdmf-common schema 1.10.0, which changes ``MeaningsTable.target`` from a link to an object-reference attribute (``dtype`` with ``reftype: object``, ``target_type: VectorData``). Files written with the hdmf-common 1.9.0 ``MeaningsTable`` (a link named "target") are still read correctly via a backwards-compatibility mapping in ``MeaningsTableMap``. There is no change in the read/write API; the change is limited to the representation on disk. @rly [#1525](https://github.com/hdmf-dev/hdmf/pull/1525) - ``DynamicTable.get_meanings_for_column`` (and the ``AlignedDynamicTable`` override) now returns ``None`` when the named column exists but has no ``MeaningsTable``, and raises ``KeyError`` only when the column itself does not exist. @rly [#1538](https://github.com/hdmf-dev/hdmf/pull/1538) - `HDMFIO.__del__` emits a `ResourceWarning` for an IO that was not closed instead of calling `close()`. Still-open IOs are flushed and closed by an `atexit` handler on the main thread. @rly [#1547](https://github.com/hdmf-dev/hdmf/pull/1547) +- `DynamicTable.add_row` now warns at most once per column that the column has become ragged, on the row that makes it ragged, instead of on that row and every row after it. The message is identical every time and a ragged column never becomes unragged, so the repeats carried no new information, and Python's default warning filter already collapsed them for most users. @h-mayorquin [#1559](https://github.com/hdmf-dev/hdmf/issues/1559) ### Fixed +- Fixed `DynamicTable.add_row` being quadratic in the number of rows when `check_ragged` is on, which is the default. The check ran `is_ragged` over the whole column after appending a single value, so row *n* cost O(n) and filling a table cost O(n^2). The appended value is now compared against the first element of the column instead, and the column is rescanned once when it grew by something other than that single append, such as a row added with `check_ragged=False`. Filling 16,000 rows into a one-column table drops from 22 s to 0.3 s. @h-mayorquin [#1559](https://github.com/hdmf-dev/hdmf/issues/1559) - Fixed a deadlock when exporting a Zarr file to HDF5 with `HDF5IO.export`. A zarr array is now read into memory before the HDF5 write (in `__list_fill__`/`__scalar_fill__`), so the read does not run while h5py's global lock is held. @rly [#1547](https://github.com/hdmf-dev/hdmf/pull/1547) - Fixed `IndexError` when selecting an empty region from an in-memory `DynamicTableRegion` (e.g. `table["region"][i]` for a ragged region row that references no target rows, or an empty slice), both when the target table's columns hold their data as numpy arrays and when the target table has ragged columns. @h-mayorquin [#1549](https://github.com/hdmf-dev/hdmf/pull/1549) - Fixed the Jupyter HTML representation (`_repr_html_`) rendering a scalar numpy `bool` or `int` attribute (e.g. `np.bool_`, `np.int64`, as read back from an HDF5 attribute) as an expandable "array" block reporting `Shape: ()`, while `float` and `str` scalars rendered inline. `_unwrap_scalar` now also unwraps numpy scalars (`np.generic`) so every scalar renders inline consistently. @h-mayorquin [#1546](https://github.com/hdmf-dev/hdmf/pull/1546) diff --git a/src/hdmf/common/table.py b/src/hdmf/common/table.py index ce18194a4..4b473f917 100644 --- a/src/hdmf/common/table.py +++ b/src/hdmf/common/table.py @@ -394,6 +394,10 @@ def __init__(self, **kwargs): # noqa: C901 # map name to column specification self.__uninit_cols = dict() + # state that keeps the ragged check in add_row O(1) instead of a rescan of the whole column + self.__ragged_columns = set() # columns found ragged, which are never checked again + self.__elements_checked_per_column = dict() # column name to how many of its elements were checked + # All tables must have ElementIdentifiers (i.e. a primary key column) # Here, we figure out what to do for that user_provided_ids = (id is not None) @@ -849,12 +853,36 @@ def add_row(self, **kwargs): col.add_vector(data[colname]) else: col.add_row(data[colname]) - if check_ragged and is_ragged(col.data): + if check_ragged and self.__becomes_ragged(colname, col): warn(("Data has elements with different lengths and therefore cannot be coerced into an " "N-dimensional array. Use the 'index' argument when creating a column to add rows " "with different lengths."), stacklevel=3) + def __becomes_ragged(self, colname, col): + """ + Whether the value just appended is the one that makes this column ragged. + + A column is warned about once, on the row that makes it ragged: the message is the same for + every later row and a ragged column never becomes unragged. ``is_ragged`` answers this by + rescanning the whole column on every row, which makes add_row quadratic, so the appended + value is compared against the first element of the column instead. Everything between them + was checked when it was added, unless the column grew by something other than this one + append, which the count of checked elements catches and answers with a single rescan. + """ + data = col.data + if colname in self.__ragged_columns or not isinstance(data, (list, tuple)): + return False # is_ragged is False for anything that is not a list or a tuple + checked = self.__elements_checked_per_column.get(colname, 0) + self.__elements_checked_per_column[colname] = len(data) + if len(data) != checked + 1: + ragged = is_ragged(data) # data did not get here through a single append, so rescan it once + else: + ragged = is_ragged([data[0], data[-1]]) + if ragged: + self.__ragged_columns.add(colname) + return ragged + def __eq__(self, other): """Compare if the two DynamicTables contain the same data. diff --git a/tests/unit/common/test_table.py b/tests/unit/common/test_table.py index c724e3a3e..d4c483ce4 100644 --- a/tests/unit/common/test_table.py +++ b/tests/unit/common/test_table.py @@ -1,10 +1,14 @@ from collections import OrderedDict import h5py +import itertools import numpy as np import os import pandas as pd import unittest +import warnings +from unittest.mock import patch +import hdmf.common.table from hdmf import Container from hdmf import TermSet, TermSetWrapper from hdmf.backends.hdf5 import H5DataIO, HDF5IO @@ -20,7 +24,7 @@ get_manager, SimpleMultiContainer) from hdmf.testing import TestCase, H5RoundTripMixin, remove_test_file -from hdmf.utils import StrDataset +from hdmf.utils import StrDataset, is_ragged from hdmf.data_utils import DataChunkIterator from tests.unit.helpers.utils import ( @@ -531,6 +535,73 @@ def test_add_row_without_required_index_and_no_ragged_check(self): table.add_row(foo=5, bar=50.0, baz='lizard', qux=[1, 2, 3]) table.add_row(foo=5, bar=50.0, baz='lizard', qux=[1, 2, 3 ,4], check_ragged=False) + def test_add_row_ragged_check_matches_is_ragged(self): + """ + For a column built one row at a time, the warning must fire on exactly the row where + is_ragged turns true and on no row after it, whatever the shapes of the values. + """ + # one element per class is_ragged distinguishes: a scalar and a string, which both count as + # length 1, an empty sequence, sequences of length 1 and 2, a tuple, and a nested element that + # is ragged inside itself + element_shapes = (1, 'ab', [], [1], [1, 2], (1, 2), [[1], [2, 3]]) + for combination in itertools.product(element_shapes, repeat=3): + column = VectorData(name='qux', description='qux column', data=[]) + table = DynamicTable(name='table', description='table description', columns=[column]) + data = [] + was_ragged = False + for element in combination: + data.append(element) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + table.add_row(qux=element) + warned = any('different lengths' in str(warning.message) for warning in caught) + self.assertEqual(warned, is_ragged(data) and not was_ragged, msg=str(data)) + was_ragged = is_ragged(data) + + def test_add_row_ragged_check_sees_elements_it_did_not_check(self): + """ + Elements the check never saw, because the column already had data or because a row was added + with check_ragged=False, still count towards raggedness for the rows after them. + """ + msg = ("Data has elements with different lengths and therefore cannot be coerced into an N-dimensional " + "array. Use the 'index' argument when creating a column to add rows with different lengths.") + # a column built with data before the first add_row is checked against all of it, and here the + # appended value matches the first element, so only the data already in the column makes it ragged + table = DynamicTable(name='table', description='table description', + columns=[VectorData(name='qux', description='qux column', data=[[1], [2, 3]])]) + with self.assertWarnsWith(UserWarning, msg): + table.add_row(qux=[4]) + + table = self.with_spec() + table.add_column(name='qux', description='qux column') + table.add_row(foo=5, bar=50.0, baz='lizard', qux=[1, 2]) + table.add_row(foo=5, bar=50.0, baz='lizard', qux=[1, 2, 3], check_ragged=False) + # again the new row matches the length of the first element, so only the skipped row makes this ragged + with self.assertWarnsWith(UserWarning, msg): + table.add_row(foo=5, bar=50.0, baz='lizard', qux=[4, 5]) + + def test_add_row_ragged_check_does_not_rescan_the_column(self): + """ + The raggedness check must cost O(1) per row: rescanning the column makes add_row quadratic. + """ + visited = 0 + original_is_ragged = hdmf.common.table.is_ragged + + def counting_is_ragged(data): + nonlocal visited + visited += len(data) if isinstance(data, (list, tuple)) else 1 + return original_is_ragged(data) + + num_rows = 1000 + table = self.with_spec() + table.add_column(name='qux', description='qux column') + with patch.object(hdmf.common.table, 'is_ragged', counting_is_ragged): + for _ in range(num_rows): + table.add_row(foo=5, bar=50.0, baz='lizard', qux=1) + + # rescanning the column would visit about num_rows ** 2 / 2 elements + self.assertLess(visited, 10 * num_rows) + def test_add_column_auto_index_int(self): """ Add a column as a list of lists after we have already added data so that we need to create a single VectorIndex From 092fa9e9c91114a2c492e63d9c4195cf0e456310 Mon Sep 17 00:00:00 2001 From: rly <310197+rly@users.noreply.github.com> Date: Tue, 18 Aug 2026 20:57:51 -0700 Subject: [PATCH 2/2] Address review of the incremental ragged check Update the check_ragged docval doc on add_row: the check no longer visits every element of the column. Expand the __becomes_ragged docstring with the invariant that makes comparing against the first element sound, and note that the count of checked elements tracks length only, so elements swapped in place through col.data are not detected. Link the CHANGELOG entries to the PR and trim them to the user-facing change. Sort the unittest.mock import into the stdlib block. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 4 ++-- src/hdmf/common/table.py | 18 +++++++++++------- tests/unit/common/test_table.py | 2 +- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 909ed50a6..7d5eda5bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,10 @@ - Added support for hdmf-common schema 1.10.0, which changes ``MeaningsTable.target`` from a link to an object-reference attribute (``dtype`` with ``reftype: object``, ``target_type: VectorData``). Files written with the hdmf-common 1.9.0 ``MeaningsTable`` (a link named "target") are still read correctly via a backwards-compatibility mapping in ``MeaningsTableMap``. There is no change in the read/write API; the change is limited to the representation on disk. @rly [#1525](https://github.com/hdmf-dev/hdmf/pull/1525) - ``DynamicTable.get_meanings_for_column`` (and the ``AlignedDynamicTable`` override) now returns ``None`` when the named column exists but has no ``MeaningsTable``, and raises ``KeyError`` only when the column itself does not exist. @rly [#1538](https://github.com/hdmf-dev/hdmf/pull/1538) - `HDMFIO.__del__` emits a `ResourceWarning` for an IO that was not closed instead of calling `close()`. Still-open IOs are flushed and closed by an `atexit` handler on the main thread. @rly [#1547](https://github.com/hdmf-dev/hdmf/pull/1547) -- `DynamicTable.add_row` now warns at most once per column that the column has become ragged, on the row that makes it ragged, instead of on that row and every row after it. The message is identical every time and a ragged column never becomes unragged, so the repeats carried no new information, and Python's default warning filter already collapsed them for most users. @h-mayorquin [#1559](https://github.com/hdmf-dev/hdmf/issues/1559) +- `DynamicTable.add_row` now warns at most once per column that the column has become ragged, on the row that makes it ragged, instead of on that row and every row after it. @h-mayorquin [#1561](https://github.com/hdmf-dev/hdmf/pull/1561) ### Fixed -- Fixed `DynamicTable.add_row` being quadratic in the number of rows when `check_ragged` is on, which is the default. The check ran `is_ragged` over the whole column after appending a single value, so row *n* cost O(n) and filling a table cost O(n^2). The appended value is now compared against the first element of the column instead, and the column is rescanned once when it grew by something other than that single append, such as a row added with `check_ragged=False`. Filling 16,000 rows into a one-column table drops from 22 s to 0.3 s. @h-mayorquin [#1559](https://github.com/hdmf-dev/hdmf/issues/1559) +- Fixed `DynamicTable.add_row` being quadratic in the number of rows when `check_ragged` is on, which is the default. Filling 16,000 rows into a one-column table drops from 22 s to 0.3 s. @h-mayorquin [#1561](https://github.com/hdmf-dev/hdmf/pull/1561) - Fixed a deadlock when exporting a Zarr file to HDF5 with `HDF5IO.export`. A zarr array is now read into memory before the HDF5 write (in `__list_fill__`/`__scalar_fill__`), so the read does not run while h5py's global lock is held. @rly [#1547](https://github.com/hdmf-dev/hdmf/pull/1547) - Fixed `IndexError` when selecting an empty region from an in-memory `DynamicTableRegion` (e.g. `table["region"][i]` for a ragged region row that references no target rows, or an empty slice), both when the target table's columns hold their data as numpy arrays and when the target table has ragged columns. @h-mayorquin [#1549](https://github.com/hdmf-dev/hdmf/pull/1549) - Fixed the Jupyter HTML representation (`_repr_html_`) rendering a scalar numpy `bool` or `int` attribute (e.g. `np.bool_`, `np.int64`, as read back from an HDF5 attribute) as an expandable "array" block reporting `Shape: ()`, while `float` and `str` scalars rendered inline. `_unwrap_scalar` now also unwraps numpy scalars (`np.generic`) so every scalar renders inline consistently. @h-mayorquin [#1546](https://github.com/hdmf-dev/hdmf/pull/1546) diff --git a/src/hdmf/common/table.py b/src/hdmf/common/table.py index 4b473f917..fd56ad953 100644 --- a/src/hdmf/common/table.py +++ b/src/hdmf/common/table.py @@ -822,7 +822,7 @@ def _add_extra_predefined_columns(self, data: dict): 'default': False}, {'name': 'check_ragged', 'type': bool, 'default': True, 'doc': ('whether or not to check for ragged arrays when adding data to the table. ' - 'Set to False to avoid checking every element if performance issues occur.')}, + 'Set to False to skip the check.')}, allow_extra=True) def add_row(self, **kwargs): """ @@ -863,12 +863,16 @@ def __becomes_ragged(self, colname, col): """ Whether the value just appended is the one that makes this column ragged. - A column is warned about once, on the row that makes it ragged: the message is the same for - every later row and a ragged column never becomes unragged. ``is_ragged`` answers this by - rescanning the whole column on every row, which makes add_row quadratic, so the appended - value is compared against the first element of the column instead. Everything between them - was checked when it was added, unless the column grew by something other than this one - append, which the count of checked elements catches and answers with a single rescan. + A column is warned about once, on the row that makes it ragged: a ragged column never becomes + unragged, so the same message on every later row carries no new information. ``is_ragged`` + answers that question by scanning the whole column, which costs O(rows) per row and makes + filling a table quadratic. Every element the check has already seen has the same length as the + first element and is not ragged within itself, so the appended value makes the column ragged + exactly when it differs from the first element, which is what this compares. The count of + elements seen guards that invariant: when the column grew by anything other than this one + append, such as a row added with ``check_ragged=False``, the column is rescanned in full once. + The count tracks length, so it does not detect elements swapped in place by code that reaches + into ``col.data`` directly. """ data = col.data if colname in self.__ragged_columns or not isinstance(data, (list, tuple)): diff --git a/tests/unit/common/test_table.py b/tests/unit/common/test_table.py index d4c483ce4..21e3821c3 100644 --- a/tests/unit/common/test_table.py +++ b/tests/unit/common/test_table.py @@ -5,8 +5,8 @@ import os import pandas as pd import unittest -import warnings from unittest.mock import patch +import warnings import hdmf.common.table from hdmf import Container