Removed unverified omn: columns from tsv format #1209
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the schema TSV (dataframe) serialization format by removing the previously emitted omn:Domain and omn:Range columns for schema attributes/properties.
Changes:
- Removed
omn:Domain/omn:Rangecolumn constants and dropped them from the required attribute/property dataframe column set. - Stopped emitting domain/range values when writing attribute entries to dataframes.
- Removed an outdated TODO about validating range/domain on read.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
hed/schema/schema_io/schema2df.py |
Drops domain/range computation for attribute rows (but currently leaves behind an unused dict literal). |
hed/schema/schema_io/df_constants.py |
Removes omn:Domain/omn:Range constants and updates attribute_columns to match the new TSV format. |
hed/schema/schema_io/df2schema.py |
Removes a stale TODO comment in attribute reading logic. |
Comments suppressed due to low confidence (1)
hed/schema/schema_io/schema2df.py:209
- Lines 205+ are now a standalone dict literal (the former
hed_id_mapping) whose values call_get_object_id(...), but the result is unused. This looks like an accidental edit and adds dead code / unnecessary work; it should either be removed entirely or restored to an assigned variable if still needed elsewhere.
{
"HedTag": self._get_object_id("HedTag", include_prefix=True),
"HedUnit": self._get_object_id("HedUnit", include_prefix=True),
"HedUnitClass": self._get_object_id("HedUnitClass", include_prefix=True),
"HedUnitModifier": self._get_object_id("HedUnitModifier", include_prefix=True),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct_columns = [hed_id, name, attributes, subclass_of, dcdescription] | ||
| tag_columns = [hed_id, name, level, subclass_of, attributes, dcdescription] | ||
| unit_columns = [hed_id, name, subclass_of, has_unit_class, attributes, dcdescription] | ||
| attribute_columns = [hed_id, name, type, domain, range, properties, dcdescription] # For the annotation property | ||
| attribute_columns = [hed_id, name, type, properties, dcdescription] # For the annotation property | ||
| property_columns = [hed_id, name, type, dcdescription] |
There was a problem hiding this comment.
This change removes the omn:Domain/omn:Range columns from the TSV schema format, but there doesn't appear to be any test asserting the updated column set (e.g., ensuring generated dataframes/TSVs no longer include these columns). Adding a regression test would help prevent these columns from being reintroduced or required again unintentionally.
1 participant
No description provided.