Skip to content

Address the various blank and incomplete tsv lines - #1502

Open
VisLab wants to merge 6 commits into
hed-standard:mainfrom
VisLab:address_issues
Open

VisLab wants to merge 6 commits into
hed-standard:mainfrom
VisLab:address_issues

Conversation

@VisLab

@VisLab VisLab commented Nov 13, 2026

Copy link
Copy Markdown
Member

No description provided.

VisLab and others added 4 commits November 13, 2026 13:28
A curly-brace reference can be n/a or blank in a row (spec 3.2.9.3
rules 3 and 10), so the sidecar check now also tries each reference
removed and reports TAG_EXPRESSION_REPEATED for those combinations.
{HED} has the removal as its only substitution, so strings that
reference it are checked again. A repeat found by several combinations
is reported once.
A .tsv line after the first that is empty, or has a different number
of tab-separated cells than the first line, is now TABULAR_INVALID
(spec 3.2.10, Appendix B reasons e and f), reported by its file line
in stage 0 so the rows are not validated. Lines end at LF or CRLF.
The file still loads, records the lines in BaseInput.line_problems,
and refuses assemble. Pins spec_tests/hed-tests to 0a07fe0.

Closes hed-standard#1475.
A tabular file is validated only when it has a HED column or a sidecar
with HED annotations (specification 3.2.10). Pins the existing behavior.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
pandas finds no columns in a file that starts with two or more empty
lines, and the file was then treated as empty before its column names
were set. The first line is one blank column name (specification
3.2.10), so it is now reported with the empty lines and cell counts.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@VisLab VisLab added bug Something isn't working python Pull requests that update Python code testing required-update labels Nov 13, 2026
@VisLab
VisLab requested a balanced review from Copilot November 13, 2026 21:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Reference expansion generates exponentially many redundant combinations and materializes the entire Cartesian product, making small sidecars expensive to validate and risking memory exhaustion.

4 open issues
What changed in this PR

This PR strengthens HED tabular validation by reporting blank and incomplete TSV lines before validating annotations. It also expands sidecar repeated-expression checks to include removed references.

Changes:

  • Record empty and wrong-width TSV lines, report their file locations, and block assembly.
  • Check sidecar substitutions with references removed and deduplicate repeated-expression reports.
  • Add regression tests and document the validation changes.
File Description
tests/​validator/​test_spreadsheet_validator.py Tests line errors, stage ordering, and file-line reporting.
tests/​validator/​test_sidecar_validator.py Tests removed-reference substitutions and repeat deduplication.
tests/​tools/​bids/​test_bids_file_group.py Tests malformed lines with invalid sidecars and HED eligibility.
tests/​models/​test_base_input.py Covers line scanning, loading, and assembly refusal.
tests/​data/​validator_tests/​bids_events_invalid_columns.tsv Adds a missing trailing cell to preserve fixture intent.
hed/​validator/​spreadsheet_validator.py Reports line problems during initial validation.
hed/​validator/​sidecar_validator.py Checks removal substitutions and deduplicates repeat issues.
hed/​tools/​bids/​bids_file_group.py Allows line errors to be reported despite sidecar errors.
hed/​models/​base_input.py Records malformed lines and prevents their assembly.
hed/​errors/​error_types.py Adds internal codes for line errors.
hed/​errors/​error_messages.py Adds line-specific validation messages.
CHANGELOG.md Documents TSV and sidecar validation changes.

Review effort: Balanced

Comment thread hed/validator/sidecar_validator.py Resolved
Comment thread hed/validator/sidecar_validator.py Resolved
Comment thread hed/models/base_input.py Resolved
Comment thread hed/models/base_input.py Resolved
Each reference name is one substitution, since replace_ref replaces
every occurrence; 20 occurrences of one reference made 3**20
combinations. The combinations are generated lazily, filled ones
first, instead of sorting the whole product in memory.
Only CR directly before LF ends a line. find_line_problems no longer
strips a CR from the last line, and pandas reads with an LF line
terminator, so its rows match the line check and a lone CR no longer
shifts later row numbers.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Newline-only named tables lose their blank header during fallback, leaving blank_columns empty and incorrectly bypassing TABULAR_INVALID (c) validation even with an annotating sidecar.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Preserve blank headers for newline-only named tables

hed/​models/​base_input.py:912

For a named table containing only LF or CRLF, this early return discards the recorded blank header, leaving blank_columns empty and skipping TABULAR_INVALID (c), even with an annotating sidecar. Preserve header_cells through the fallback for named input; genuinely empty text is already handled above. Update the newline-only expectation in tests/models/test_base_input.py:568-569 to expect a blank column.

4 resolved since last review

Review effort: Balanced

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

bug Something isn't working python Pull requests that update Python code required-update testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants