Fixing the required tag cleanup

hed/schema/schema_comparer.py

@@ -805,7 +805,9 @@ def _check_other_attributes(self, change_dict, section_key, entry1, entry2):
             unique_inherited_keys = unique_keys
         all_unique_keys = unique_keys.union(unique_inherited_keys).difference(already_checked_attributes)
 
-        for key in all_unique_keys:
+        # Sorted: a set of strings iterates in an order that changes with the hash seed, and the
+        # changelog this feeds is a generated, committed file.
+        for key in sorted(all_unique_keys):
             is_inherited = key in unique_inherited_keys
             is_direct = key in unique_keys
 
@@ -857,7 +859,7 @@ def _add_extras_changes(self, change_dict):
         extras2 = getattr(self.schema2, "extras", {}) or {}
 
         all_keys = set(extras1.keys()).union(extras2.keys())
-        for key in all_keys:
+        for key in sorted(all_keys):
             df1 = extras1.get(key)
             df2 = extras2.get(key)
             # An absent section and one present with no rows are the same thing: writers always
@@ -1001,6 +1003,11 @@ def _compare_dataframes(df1, df2, key_cols):
                     * "Row missing in second schema"
                     * "Duplicate keys found"
                     * "Column values differ"
+
+        Notes:
+            - The differences are listed in key order (as text), so two runs on the same frames give the
+              same list. A set of keys iterates in an order that changes with the hash seed, which put
+              spurious reorderings into the generated changelog.
         """
         results = []
 
@@ -1009,7 +1016,10 @@ def _compare_dataframes(df1, df2, key_cols):
 
         all_keys = set(df1_indexed.index).union(df2_indexed.index)
 
-        for key in all_keys:
+        def key_text(key):
+            return [str(part) for part in key] if isinstance(key, tuple) else [str(key)]
+
+        for key in sorted(all_keys, key=key_text):
             if key not in df1_indexed.index:
                 results.append({"row": key, "cols": None, "message": "Row missing in first schema"})
             elif key not in df2_indexed.index:

hed/validator/hed_validator.py

@@ -94,14 +94,16 @@ def run_basic_checks(self, hed_string, allow_placeholders) -> list[dict]:
         issues += self._def_validator.validate_def_tags(hed_string)
         return issues
 
-    def run_full_string_checks(self, hed_string, whole_row=True) -> list[dict]:
+    def run_full_string_checks(self, hed_string, *, required_tags=True, unique_tags=True) -> list[dict]:
         """Run all full-string validation checks on a HED string.
 
         Parameters:
             hed_string (HedString): The HED string to validate.
-            whole_row (bool): If False, the string is one cell of a row whose other columns also carry HED,
-                so the checks that need every tag of the row (required and unique tags) are skipped. The
-                group-level checks still apply: a group in the cell is a group of the row.
+            required_tags (bool): Run the required-tag check. Pass False for a fragment of a row (a HED cell,
+                a sidecar string): the tag it lacks may come from another column, so the check needs the
+                assembled row.
+            unique_tags (bool): Run the unique-tag check. A unique tag repeated inside a fragment is repeated
+                in the row too, so this check is safe on a fragment; pass False to leave it to assembly.
 
         Returns:
             list[dict]: A list of issues found during validation. Each issue is represented as a dictionary.
@@ -111,14 +113,25 @@ def run_full_string_checks(self, hed_string, whole_row=True) -> list[dict]:
             - Each check is a callable function that takes `hed_string` as input and returns a list of issues.
             - If any check returns issues, the method stops and returns those issues immediately.
             - If no issues are found, an empty list is returned.
+            - The group-level checks (tag placement, reserved groups, duplicates, Onset and Offset) always
+              run: a group in a fragment is a group of the row.
 
         """
+
+        def row_tag_checks(string_obj):
+            tags = string_obj.get_all_tags()
+            issues = []
+            if required_tags:
+                issues += self._group_validator.check_for_required_tags(tags)
+            if unique_tags:
+                issues += self._group_validator.check_multiple_unique_tags_exist(tags)
+            return issues
+
         checks = [
+            row_tag_checks,
             self._group_validator.run_tag_level_validators,
             self._def_validator.validate_onset_offset,
         ]
-        if whole_row:
-            checks.insert(0, self._group_validator.run_all_tags_validators)
 
         for check in checks:
             issues = check(hed_string)  # Call each function with `hed_string`

hed/validator/sidecar_validator.py

@@ -123,8 +123,11 @@ def _validate_sidecar(self, sidecar, extra_def_dicts, error_handler) -> list[dic
                             modified_string = df_util.replace_ref(modified_string, f"{{{ref}}}", ref_dict[ref])
                         hed_string_obj = HedString(modified_string, hed_schema=self._schema, def_dict=sidecar_def_dict)
 
+                        # A sidecar string is a fragment of a row: a tag it lacks may come from another
+                        # column, so the required-tag check waits for assembly. A unique tag repeated in
+                        # the fragment is repeated in the row, so that check runs here.
                         error_handler.push_error_context(ErrorContext.HED_STRING, hed_string_obj)
-                        new_issues += hed_validator.run_full_string_checks(hed_string_obj)
+                        new_issues += hed_validator.run_full_string_checks(hed_string_obj, required_tags=False)
                         error_handler.add_context_and_filter(new_issues)
                         issues += new_issues
                         error_handler.pop_error_context()  # Hed string

hed/validator/spreadsheet_validator.py

@@ -36,14 +36,15 @@ class SpreadsheetValidator:
 
     The stages run in order and each stops the run when it finds an error (never on a warning alone):
 
-    1. **Sidecar**: every template and categorical string of the sidecar (``SidecarValidator``).
+    1. **Sidecar**: every template and categorical string of the sidecar (``SidecarValidator``). A sidecar
+       string is a fragment of a row, so the required-tag check waits for assembly.
     2. **Column values**: the column structure (mapper issues, sidecar references to columns the table
        lacks), then each value column's distinct values against the units or value class of its ``#``
        tag, and each categorical column's distinct values against its sidecar keys.
     3. **HED column**: each distinct string of every HED-tags column, the basic (single-string) checks.
        For a source that offers no assembly, the group-level checks follow on the same strings, since no
-       stage 4 will run them on the whole row. The required-tag and unique-tag checks belong to the
-       assembled row and never run on a cell.
+       stage 4 will run them on the whole row. The required-tag check belongs to the assembled row and
+       never runs on a cell; the unique-tag check does, since a repeat inside a cell is a repeat of the row.
     4. **Assembly**: the rows assembled from all columns, the group-level checks, and the onset and
        temporal checks of a timeline file.
 
@@ -229,7 +230,7 @@ def _validate_source(self, source, sidecar, extra_def_dicts, error_handler, row_
         # Stage 4: the assembled rows, only for a source that can provide them. The source is asked only
         # now, so a stage 3 error never costs a table conversion. A source that offers no assembly gets
         # the group-level checks on each distinct HED cell instead, since no stage 4 will run them on the
-        # whole row; the required-tag and unique-tag checks stay with the assembled row.
+        # whole row; the required-tag check stays with the assembled row.
         base_input = source.as_base_input()
         if base_input is not None:
             issues += self.validate_assembled(base_input, error_handler, row_offset=row_offset)
@@ -383,8 +384,9 @@ def validate_hed_column(self, column_name, distinct, error_handler, row_offset=0
             row_offset (int): Added to the first row of each reported string.
             full_string (bool): If True, also run the group-level checks on each string that passes the
                 basic checks. For a caller that will never assemble rows; otherwise those checks belong
-                to the assembly stage, where the row is whole. The required-tag and unique-tag checks
-                need the assembled row and never run here.
+                to the assembly stage, where the row is whole. The required-tag check needs the assembled
+                row and never runs here; the unique-tag check does, since a repeat inside a cell is a repeat
+                of the row.
 
         Returns:
             list[dict]: The issues found, each with ``row_count``.
@@ -394,18 +396,18 @@ def validate_hed_column(self, column_name, distinct, error_handler, row_offset=0
             hed_string = HedString(text, self._schema)
             string_issues = self._hed_validator.run_basic_checks(hed_string, allow_placeholders=False)
             if full_string and not check_for_any_errors(string_issues):
-                string_issues += self._hed_validator.run_full_string_checks(hed_string, whole_row=False)
+                string_issues += self._hed_validator.run_full_string_checks(hed_string, required_tags=False)
             issues += self._report_distinct(string_issues, hed_string, rows, column_name, error_handler, row_offset)
         return issues
 
     def _validate_hed_cells(self, column_name, distinct, error_handler, row_offset) -> list[dict]:
         """Run the group-level checks on each distinct string of a HED-tags column that has passed stage 3,
         for a source that offers no assembly. One more parse per distinct string. A cell is part of a row,
-        so the required-tag and unique-tag checks are left to assembly."""
+        so the required-tag check is left to assembly; a unique tag repeated inside the cell is reported."""
         issues = []
         for text, rows in distinct.items():
             hed_string = HedString(text, self._schema)
-            string_issues = self._hed_validator.run_full_string_checks(hed_string, whole_row=False)
+            string_issues = self._hed_validator.run_full_string_checks(hed_string, required_tags=False)
             issues += self._report_distinct(string_issues, hed_string, rows, column_name, error_handler, row_offset)
         return issues
 

tests/schema/test_schema_compare.py

@@ -326,6 +326,32 @@ def test_row_added_to_schema2_is_minor(self):
         self.assertEqual(len(added), 1)
         self.assertEqual(added[0]["change_type"], "Minor")
 
+    def test_one_sided_rows_listed_in_key_order(self):
+        """Rows present in one schema only are recorded in key order, in either direction."""
+        base = pd.DataFrame({"source": ["src1"], "link": ["http://a.org"], "description": ["d"]})
+        more = pd.DataFrame(
+            {
+                "source": ["src1", "src9", "src3", "src5"],
+                "link": ["http://a.org", "http://i.org", "http://c.org", "http://e.org"],
+                "description": ["d", "i", "c", "e"],
+            }
+        )
+        schema1 = self._make_schema({SOURCES_KEY: base.copy()})
+        schema2 = self._make_schema({SOURCES_KEY: more.copy()})
+        changes = self._extras_result(schema1, schema2)[SOURCES_KEY]
+        self.assertEqual(
+            [c["change"] for c in changes],
+            [f"Row {key} missing in first schema" for key in ["src3", "src5", "src9"]],
+        )
+
+        schema1 = self._make_schema({SOURCES_KEY: more.copy()})
+        schema2 = self._make_schema({SOURCES_KEY: base.copy()})
+        changes = self._extras_result(schema1, schema2)[SOURCES_KEY]
+        self.assertEqual(
+            [c["change"] for c in changes],
+            [f"Row {key} missing in second schema" for key in ["src3", "src5", "src9"]],
+        )
+
     def test_row_removed_from_schema2_is_minor(self):
         """A row present in schema1 but not schema2 is Minor."""
         df1 = pd.DataFrame(
@@ -441,6 +467,23 @@ def test_multiple_key_columns(self):
         self.assertIn("Row missing in first schema", messages)
         self.assertIn("Column values differ", messages)
 
+    def test_differences_listed_in_key_order(self):
+        """The list is in key order whatever order the frames or the hash seed would give, so a generated
+        changelog does not reorder between runs."""
+        df1 = pd.DataFrame({"key": ["m", "z"], "val": ["x", "old"]})
+        df2 = pd.DataFrame({"key": ["z", "c", "a", "b"], "val": ["new", "x", "x", "x"]})
+        results = SchemaComparer._compare_dataframes(df1, df2, ["key"])
+        self.assertEqual([r["row"] for r in results], ["a", "b", "c", "m", "z"])
+        self.assertEqual(
+            [r["message"] for r in results],
+            ["Row missing in first schema"] * 3 + ["Row missing in second schema", "Column values differ"],
+        )
+
+        df1 = pd.DataFrame({"k1": ["b"], "k2": ["1"], "val": ["x"]})
+        df2 = pd.DataFrame({"k1": ["b", "a", "b"], "k2": ["1", "2", "0"], "val": ["x", "y", "z"]})
+        results = SchemaComparer._compare_dataframes(df1, df2, ["k1", "k2"])
+        self.assertEqual([r["row"] for r in results], [("a", "2"), ("b", "0")])
+
 
 class TestDerivableUnitRemoval(unittest.TestCase):
     """HED 8.5.0 drops listed SI variants (uV) that stay valid through modifiers: Patch, not Major."""

tests/validator/test_hed_validator.py

@@ -229,6 +229,28 @@ def test_duplicate_group_in_definition(self):
         issues = test_string.validate(hed_schema)
         self.assertEqual(len(issues), 1)
 
+    def test_run_full_string_checks_row_tag_flags(self):
+        """The required-tag and unique-tag checks need the whole row; each can be left out for a fragment,
+        while the group-level checks always run."""
+        schema_path = os.path.join(self.hed_base_dir, "HED8.0.0_added_tests.mediawiki")
+        validator = HedValidator(schema.load_schema(schema_path))
+
+        missing_required = HedString("Event, (Onset)", validator._hed_schema)
+        codes = [issue["code"] for issue in validator.run_full_string_checks(missing_required)]
+        self.assertEqual(codes, ["REQUIRED_TAG_MISSING", "REQUIRED_TAG_MISSING"])
+        codes = [issue["code"] for issue in validator.run_full_string_checks(missing_required, required_tags=False)]
+        self.assertEqual(codes, ["TEMPORAL_TAG_ERROR"])
+
+        repeated_unique = HedString(
+            "Action, Animal-agent, (Event-context, (Red, Blue)), (Event-context, (Green, Yellow))",
+            validator._hed_schema,
+        )
+        codes = [issue["code"] for issue in validator.run_full_string_checks(repeated_unique)]
+        self.assertEqual(codes, ["TAG_NOT_UNIQUE"])
+        self.assertEqual(validator.run_full_string_checks(repeated_unique, unique_tags=False), [])
+        codes = [issue["code"] for issue in validator.run_full_string_checks(repeated_unique, required_tags=False)]
+        self.assertEqual(codes, ["TAG_NOT_UNIQUE"])
+
     def test_severity_enum_not_string_github_hedit_161(self):
         """Regression test for critical bug: severity should be ErrorSeverity enum, not string.
 

tests/validator/test_staged_validation.py

@@ -268,9 +268,10 @@ def test_base_input_is_not_asked_for_when_an_earlier_stage_stops(self):
         self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.VALUE_INVALID])
         self.assertEqual(source.base_input_calls, 0)
 
-    def test_required_and_unique_tags_are_checked_only_on_assembled_rows(self):
-        """A HED cell is part of a row, so the required-tag and unique-tag checks never run on it alone: the
-        sidecar may supply the tags. They run in the assembly stage; the group-level checks apply to a cell."""
+    def test_required_tags_are_checked_only_on_assembled_rows(self):
+        """A HED cell is part of a row, so the required-tag check never runs on it alone: the sidecar may supply
+        the tags. It runs in the assembly stage. The group-level and unique-tag checks apply to a cell, since
+        what they find inside the cell holds for the row."""
         schema_path = os.path.join(
             os.path.dirname(os.path.realpath(__file__)), "../data/validator_tests/HED8.0.0_added_tests.mediawiki"
         )
@@ -293,16 +294,37 @@ def test_required_and_unique_tags_are_checked_only_on_assembled_rows(self):
         issues = validator.validate(ListColumnSource(columns, base_input=frame))
         self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.REQUIRED_TAG_MISSING] * 2)
 
-        # The same holds for a unique tag repeated within one cell.
+        # Required tags split between a sidecar string and the HED cell: clean through a real assembled input,
+        # so stage 1 must not hold the sidecar string alone to the required-tag rule either.
+        split = _sidecar({"condition": {"HED": {"a": "Action"}}})
+        frame = _tabular([["onset", "duration", "condition", "HED"], [1.0, 0, "a", "Animal-agent"]], sidecar=split)
+        self.assertEqual(validator.validate(frame), [])
+        self.assertEqual(
+            validator.validate(ListColumnSource({"condition": ["a"], "HED": ["Animal-agent"]}), sidecar=split), []
+        )
+
+        # A unique tag repeated inside one sidecar string is repeated in every row that uses it, so stage 1
+        # still reports it (hed-tests TAG_NOT_UNIQUE has a sidecar-only case).
+        repeated = _sidecar(
+            {"condition": {"HED": {"a": "(Event-context, (Red, Blue)), (Event-context, (Green, Yellow))"}}}
+        )
+        issues = self.validator.validate(ListColumnSource({"condition": ["a"]}), sidecar=repeated)
+        self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.TAG_NOT_UNIQUE])
+
+        # A unique tag repeated within one cell is reported without assembly, once with a row count, and
+        # with assembly once per row; never both, since the cell pass runs only when no assembly follows.
         columns = {
-            "onset": [1.0],
-            "duration": [0],
-            "HED": ["(Event-context, (Red, Blue)), (Event-context, (Green, Yellow))"],
+            "onset": [1.0, 2.0],
+            "duration": [0, 0],
+            "HED": ["(Event-context, (Red, Blue)), (Event-context, (Green, Yellow))"] * 2,
         }
-        self.assertEqual(self.validator.validate(ListColumnSource(columns)), [])
+        issues = self.validator.validate(ListColumnSource(columns))
+        self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.TAG_NOT_UNIQUE])
+        self.assertEqual((issues[0][ErrorContext.ROW], issues[0][ROW_COUNT_KEY]), (0, 2))
         frame = TabularInput(pd.DataFrame({name: [str(v) for v in values] for name, values in columns.items()}))
         issues = self.validator.validate(ListColumnSource(columns, base_input=frame))
-        self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.TAG_NOT_UNIQUE])
+        self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.TAG_NOT_UNIQUE] * 2)
+        self.assertTrue(all(ROW_COUNT_KEY not in issue for issue in issues))
 
     def test_plain_source_gets_a_warning_for_a_column_the_sidecar_does_not_describe(self):
         source = ListColumnSource({"id": [1, 2], "onset": [1.0, 2.0], "HED": ["Red", "Blue"]})