diff --git a/hed/schema/schema_comparer.py b/hed/schema/schema_comparer.py index dbaea7a1..75f61648 100644 --- a/hed/schema/schema_comparer.py +++ b/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: diff --git a/hed/validator/hed_validator.py b/hed/validator/hed_validator.py index 2420bf37..23047bca 100644 --- a/hed/validator/hed_validator.py +++ b/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` diff --git a/hed/validator/sidecar_validator.py b/hed/validator/sidecar_validator.py index 8b37d0cc..c47648a0 100644 --- a/hed/validator/sidecar_validator.py +++ b/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 diff --git a/hed/validator/spreadsheet_validator.py b/hed/validator/spreadsheet_validator.py index fd7b2fac..05246e5a 100644 --- a/hed/validator/spreadsheet_validator.py +++ b/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 diff --git a/tests/schema/test_schema_compare.py b/tests/schema/test_schema_compare.py index 9a779d94..b4277b6d 100644 --- a/tests/schema/test_schema_compare.py +++ b/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.""" diff --git a/tests/validator/test_hed_validator.py b/tests/validator/test_hed_validator.py index 4d6c50f2..2cfc213c 100644 --- a/tests/validator/test_hed_validator.py +++ b/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. diff --git a/tests/validator/test_staged_validation.py b/tests/validator/test_staged_validation.py index 491cbeb4..d90e8224 100644 --- a/tests/validator/test_staged_validation.py +++ b/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"]})