Skip to content

refactor(dynamictable): rename getRow locals and use an arguments block - #940

Open
ehennestad wants to merge 3 commits into
mainfrom
rename-getrow-variables
Open

ehennestad wants to merge 3 commits into
mainfrom
rename-getrow-variables

Conversation

@ehennestad

@ehennestad ehennestad commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

Refactor getRow so it is easier to reason about. Its variable names and docstrings were unclear or misleading.

Background — getRow builds the table that DynamicTable.getRow and toTable return: one table variable per requested column, read for the requested rows only. #909 rewrites getRow to read ragged columns in a few calls.

Problem — A developer who reads getRow, or reviews #909, has to learn what the code does from text that misdescribes it. Three things get in the way:

  • Variable and function names are unclear or misleading. A reader has to trace each name to its use to learn what it holds. The cell array that becomes the table's variables is called row. The number of subscripts used to index a column is called rank, which is also MATLAB's matrix-rank function. The helper that reads one column is called select. The row indices, the column name and the chain of index columns are abbreviated to ind, cn and colIndStack.
  • Docstrings and comments are outdated. They describe an earlier version of the function. The header says the function takes a scalar 0-based index and returns a set of output arguments. The function takes a vector of 1-based indices and returns a table. Two comments call a column vector a row vector.
  • Argument validation is split between two mechanisms. validateattributes calls check the table and the indices, and an inputParser with anonymous validators checks the options. A wrong option value is reported as It must satisfy the function: @(x)isempty(x)||iscellstr(x) rather than as the rule.

None of this changes what getRow returns, so nothing shows up for users or in CI.

Solution — Names say what a variable holds or what a function does. Docstrings and comments describe the current code. An arguments block validates the inputs, and its errors name the argument and the rule. #909, #923 and #912 are rebuilt on this branch, so their diffs read in the new vocabulary.

What changed

Implementation notes

Renames, old to new:

Old New
row columnData
ind, matInd rowIndices
cn, column_name columnName
i iColumn, iField
indexNames, colIndStack vectorChainNames
structNames compoundMemberNames
columnData (one compound member) memberData
array_size, num_rows, is_row_dim arraySize, numRows, isRowDimension
rank numSubscripts
refProp dataSize
selectInd subscripts
id, idMatch requestedIds, isIdFound
select, returning selected getColumnRows, returning columnRows
getIndById getRowIndicesById
InvalidVectorDataShapeError createInvalidShapeError

Examples

Validation messages and accepted inputs

The snippet builds a three-row table in memory and calls getRow with useId given as the number 1, with an empty index list, with a wrong columns value, and with a column vector of row indices.

dynamicTable = types.hdmf_common.DynamicTable('description', 'example', 'colnames', {'score'});
dynamicTable.addRow('id', 10, 'score', 0.1);
dynamicTable.addRow('id', 20, 'score', 0.2);
dynamicTable.addRow('id', 30, 'score', 0.3);

try
    row = types.util.dynamictable.getRow(dynamicTable, 20, 'useId', 1);
    fprintf('useId given as 1: score %g\n', row.score);
catch e
    fprintf('useId given as 1: Error: %s\n', e.message);
end
try
    row = types.util.dynamictable.getRow(dynamicTable, []);
    fprintf('no indices: table of size %dx%d\n', height(row), width(row));
catch e
    fprintf('no indices: Error: %s\n', e.message);
end
try
    row = types.util.dynamictable.getRow(dynamicTable, 1, 'columns', 42);
    fprintf('columns given as 42: %s\n', class(row));
catch e
    fprintf('columns given as 42: Error: %s\n', e.message);
end
row = types.util.dynamictable.getRow(dynamicTable, [3; 1]);
fprintf('column vector of indices: scores %s\n', mat2str(row.score.'));

Before — The first two calls stop with an error, and the third reports an anonymous function instead of the rule. Only the column vector of indices returns rows.

useId given as 1: Error: The value of 'useId' is invalid. It must satisfy the function: @(x)islogical(x).
no indices: Error: Expected input to be a vector.
columns given as 42: Error: The value of 'columns' is invalid. It must satisfy the function: @(x)isempty(x)||iscellstr(x).
column vector of indices: scores [0.3 0.1]

After — useId given as 1 selects the row with id 20, the empty index list gives a table with no rows and the one requested column, and the wrong columns value is reported with the rule in words. The column vector gives the same rows as before.

useId given as 1: score 0.2
no indices: table of size 0x1
columns given as 42: Error: Invalid value for 'columns' argument. Value must be empty or a cell array of character vectors.
column vector of indices: scores [0.3 0.1]

How to test

Run the snippet above on this branch. The output should match the After block.

Checklist

  • Have you ensured the PR description clearly describes the problem and solutions?
  • Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
  • If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

🤖 Generated with Claude Code

ehennestad and others added 3 commits October 6, 2026 22:31
The cell array that becomes the table's variables was named row, and
several other names were abbreviations, snake_case, or shadowed a
MATLAB function:

- row -> columnData; ind and matInd -> rowIndices; cn -> columnName;
  the loop counters i -> iColumn and iField
- indexNames and colIndStack -> vectorChainNames, the column followed
  by the VectorIndex columns after it
- structNames -> compoundMemberNames; columnData in select ->
  memberData, so the name is not reused for a compound member
- array_size, num_rows, is_row_dim -> arraySize, numRows,
  isRowDimension
- rank, which shadows the matrix rank function, -> numSubscripts;
  refProp -> dataSize; selectInd -> subscripts
- id, idMatch, column_name -> requestedIds, isIdFound, columnName
- select -> getColumnRows, returning columnRows instead of selected;
  getIndById -> getRowIndicesById; InvalidVectorDataShapeError ->
  createInvalidShapeError, whose message now reads "does not match"

Comments and error identifiers are unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lock

Replace the validateattributes calls and the inputParser with an
arguments block. The table is validated by the existing
matnwb.common.validation.mustBeDynamicTable, which accepts both the
hdmf_common and the legacy core class. The two name lists keep the
same rule as the old anonymous validators, empty or a cellstr, through
a local validator, since no validator for that exists in
matnwb.common.compatibility and mustBeText is newer than R2019b. The
default categories come from a local function, as a default expression
cannot branch on the table's class.

Behaviour that changes: a column vector of row indices is reshaped to
a row by the (1,:) size, which the public getRow methods already
enforce; an empty index list is accepted and returns a table without
rows; a numeric useId is converted to logical. Option names still
match case-insensitively and partially, as with inputParser.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The header described a scalar 0-based index, an `id` keyword and a set
of output arguments. The function takes a vector of 1-based row indices,
or id values with the useId flag, and returns a table. Two comments in
the column read called a column vector a row vector, the DataPipe
permute kept a loose paragraph about "non-row vectors", and the N-d
branch claimed to put the last dimension first where it moves whichever
dimension spans the rows.

Also explain the VectorIndex chain a column is read through, why a
single array row is cell-wrapped, which columns reach the compound
struct split, and give getRowIndicesById and validateRowIndices H1
lines.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@ehennestad
ehennestad added this pull request to stack #941 October 6, 2026 20:45
@codecov

codecov Bot commented Oct 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.95652% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.44%. Comparing base (ff0d00e) to head (0bb63d3).

Files with missing lines Patch % Lines
+types/+util/+dynamictable/getRow.m 86.95% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #940      +/-   ##
==========================================
- Coverage   95.46%   95.44%   -0.03%     
==========================================
  Files         240      240              
  Lines        8869     8870       +1     
==========================================
- Hits         8467     8466       -1     
- Misses        402      404       +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ehennestad
ehennestad requested a review from bendichter October 6, 2026 21:31
@ehennestad ehennestad added this to the v2.12.0 milestone Oct 7, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant