Improve handling of hedIds

[VisLab]

Several of the functions that handle, create, and insert hedIds have TODOs that need to be addressed before the release of 1.0.0.

[VisLab]

Should also have an ID range checker that makes sure that the IDs assigned are in the correct ranges.

[VisLab]

@claude do a thorough review of hedId handing --- Do a summary and recommend updates and additional tests.

[github-actions]

Claude finished @VisLab's task in 5m 15s —— View job

---

HedId Handling Review

Tasks:

• Gather context and understand the request
• Find all hedId-related code files
• Review hedId creation/insertion functions
• Review hedId validation/range checking
• Review existing tests
• Summarize findings and recommendations

---

Files Reviewed

File	Role
hed/schema/schema_io/hed_id_util.py	ID range lookup, ID assignment, DataFrame verification & merge
hed/schema/schema_validation/hed_id_validator.py	Cross-version consistency & range validation
hed/schema/schema_io/df_constants.py	Constants: section keys, column names, struct base IDs
hed/schema/schema_io/schema2df.py	Schema → DataFrame conversion (writes hedId attribute)
hed/schema/schema_io/df_util.py	DataFrame loading/saving, get_library_name_and_id
hed/schema/schema_validation/compliance.py	Compliance checker that invokes HedIDValidator
hed/schema/hed_cache.py	get_library_data() — source of truth for valid ID ranges
hed/scripts/add_hed_ids.py / hed_convert_schema.py	CLI entry points for adding/updating IDs
tests/schema/test_hed_id_util.py	Unit tests for hed_id_util
tests/schema/test_schema_validator_hed_id.py	Unit tests for HedIDValidator

---

Critical Bugs

1. Verification loop is a no-op — wrong constant used (hed_id_util.py:93)

for df_key, df in dataframes.items():
    if df_key in constants.DF_SUFFIXES:   # ← BUG: should be DF_EXTRAS
        continue

DF_SUFFIXES contains all standard schema keys (Tag, Structure, Unit, UnitClass, ValueClass, AnnotationProperty, …). Every key in dataframes matches, so the loop body is never reached. The ID conflict check before assign_missing_ids is effectively disabled.

The intent is to skip the extra sections (Prefixes, ExternalAnnotations, Sources) that carry no hedIds. The correct constant is DF_EXTRAS:

if df_key in constants.DF_EXTRAS:
    continue

Because of this bug, the try/except HedFileError block in test_update_dataframes_from_schema that asserts len(e.issues) == 115 never fires — the test passes vacuously.

---

2. assign_hed_ids_section doesn't modify the DataFrame (hed_id_util.py:217)

for _row_number, row in df.iterrows():
    ...
    row[constants.hed_id] = hed_id   # ← modifies a copy, NOT df

df.iterrows() yields copies of each row as a Series. Assigning into row does not write back to df. IDs are silently discarded and the DataFrame is left unchanged.

The test test_assign_hed_ids_section calls assign_hed_ids_section(df, …) and then checks df.equals(expected_result), but this assertion passes only if pandas happens to share memory internally (unreliable; fails under pandas 2.x Copy-on-Write).

Fix:

for _row_number, row in df.iterrows():
    if row[constants.hed_id]:
        continue
    df.at[_row_number, constants.hed_id] = f"HED_{sorted_unused_ids.pop():07d}"

---

Logic Errors

3. Dead-code early return in verify_tag_id (hed_id_validator.py:99)

new_id = tag_entry.attributes.get(attribute_name, "")
...
if new_id:
    new_id = int(new_id.removeprefix("HED_"))   # int or stays ""
# Nothing to verify
if new_id is None and old_id is None:           # ← new_id is NEVER None
    return []

tag_entry.attributes.get(…, "") returns "", not None, when the attribute is absent. After the if new_id: block, new_id is either int or "" — never None. The guard is dead code and the "nothing to verify" case falls through into the comparison and range checks.

Fix:

if not new_id and old_id is None:
    return []

---

4. Unknown library returns empty range, making all IDs fail (hed_id_util.py:42-56)

When get_library_data(schema_name) returns {} (library not in library_data.json), _get_hedid_range returns set(). Inside _verify_hedid_matches:

if id_int not in unused_tag_ids:   # always True for empty set → error for every ID

This means a schema for an unregistered library cannot have any hedIds without generating errors. The range check should be skipped when no library data is available:

if not unused_tag_ids:
    continue  # skip range check for unknown libraries

---

5. min()/max() crash on empty set (hed_id_util.py:183)

f"Valid range is: {min(unused_tag_ids)} to {max(unused_tag_ids)}"

If unused_tag_ids is empty (caused by bug #4 above), min() and max() raise ValueError. This is a secondary crash triggered by the same root cause.

---

6. HedFileError raises with only first error code (hed_id_util.py:103-110)

raise HedFileError(
    hedid_errors[0]["code"],   # ← only first code, ignoring other error types
    f"{len(hedid_errors)} issues found …",
    …
)

If the batch of errors contains multiple distinct error codes (e.g., a format error and a mismatch error), only the first code is surfaced. All issues are passed via issues=hedid_errors so callers can inspect them, but the exception's primary code is misleading for mixed error lists.

---

Missing Features / TODOs

7. No completeness check for hedIds (hed_id_validator.py:74)

# todo: If you have a way to know the schema should have 100% ids,
#       you could check for that and flag missing

There is no mechanism to require that every entry in a schema section has a hedId. This matters pre-release: a newly added tag that forgets its ID will silently pass validation. Completeness checking (warn/error if any entry in the section lacks a hedId) should be added, gated by a flag like require_complete_ids.

8. No duplicate hedId detection

The compliance checker validates format and range of individual IDs but never verifies that the same numeric ID appears at most once across all sections. A duplicate ID would be silently accepted. A pass over all IDs (collected via get_all_ids per section) with a collision check should be added to check_compliance.

9. OSError silently swallowed on file load (df_util.py:252)

except OSError:
    # todo: consider if we want to report this error(we probably do)
    pass

A missing or unreadable TSV file produces an empty DataFrame without any warning. This can cause confusing downstream errors.

10. Robustness gap for partial schema files (hed_convert_schema.py:50)

# todo: We need a more robust system for if some files are missing
#       (especially for library schemas which will probably lack some)
if any(value is None for value in source_dataframes.values()):
    source_dataframes = schema.get_as_dataframes()

The fallback discards any existing extra columns from the ontology TSV, defeating the point of the merge step.

11. merge_dfs is O(n × m) (hed_id_util.py:227-235)

# todo: vectorize this at some point
for _index, row in source_df.iterrows():
    match_index = dest_df[dest_df["rdfs:label"] == row["rdfs:label"]].index

Should be replaced with a pandas merge/join keyed on rdfs:label.

---

Test Gaps

12. test_verify_tag_id is incomplete (test_schema_validator_hed_id.py:44-56)

event_entry.attributes[HedKey.HedID] = "HED_XXXXXXX"
# ← verify_tag_id is never called again!
self.assertTrue("It must be an integer in the format", issues[0]["message"])

The second self.assertTrue checks issues from the first call (wrong ID range), not from the non-integer case. The invalid-format path is never actually exercised. The test should make a second call and check the returned issues.

13. Missing test coverage

The following scenarios lack any test cases:

• Duplicate hedId within a section or across sections
• Completeness check (entry missing a hedId when all others have one)
• Unknown library (no entry in library_data.json) — should it skip range validation or error?
• assign_hed_ids_section — verify the DataFrame is actually mutated (currently broken, see bug Updates from JavaScript version #2)
• update_dataframes_from_schema — verify hedId conflicts are actually detected (currently a no-op, see bug Consolidate validator #1)
• Range verification for library schemas (score, lang, slam, mouse)
• verify_tag_id with non-integer ID format (HED_XXXXXXX)
• verify_tag_id for a schema with no previous version (first release)

---

Recommended Updates (Priority Order)

Priority	File	Change
Critical	hed_id_util.py:93	Change DF_SUFFIXES → DF_EXTRAS in the skip condition
Critical	hed_id_util.py:211-217	Replace row[col] = val with df.at[row_num, col] = val
High	hed_id_validator.py:99	Fix dead-code guard: not new_id and old_id is None
High	hed_id_util.py:43-56	Skip range check when unused_tag_ids is empty
High	hed_id_util.py:183	Guard min()/max() call against empty set
Medium	compliance.py	Add duplicate hedId check across sections
Medium	hed_id_validator.py:74	Implement completeness check (configurable)
Medium	df_util.py:252	Surface OSError as a warning instead of silencing it
Low	hed_id_util.py:227	Vectorize merge_dfs with a pandas merge on rdfs:label
Low	hed_convert_schema.py:50	Improve partial-file fallback to preserve extra columns

---

Additional Tests to Add

test_hed_id_util.py:
  TestVerifyHedIdMatches.test_verify_unknown_library        # empty range → skip, not error
  TestVerifyHedIdMatches.test_duplicate_hed_id_in_section   # same ID used twice
  TestVerifyHedIdMatches.test_empty_unused_ids_no_crash     # min()/max() guard

  TestAssignHedIds.test_assign_actually_mutates_df          # DataFrame is modified in-place
  TestAssignHedIds.test_assign_preserves_existing           # existing IDs not overwritten
  TestAssignHedIds.test_assign_with_exhausted_pool          # no IDs left to assign

  TestUpdateDataframes.test_conflict_detected               # verify HedFileError IS raised
  TestUpdateDataframes.test_unknown_library_skips_range     # no false-positive for unknown lib

test_schema_validator_hed_id.py:
  Test.test_verify_tag_id_invalid_format                    # "HED_XXXXXXX" → error
  Test.test_verify_tag_id_no_previous_version               # first-release schema → no error
  Test.test_verify_tag_id_missing_id_completeness           # entry without ID flagged (if feature added)

test_schema_compliance.py (or new file):
  TestDuplicateHedIds.test_duplicate_within_tags_section
  TestDuplicateHedIds.test_duplicate_across_sections

[VisLab]

These suggested were addressed in PR #1266.