From 2d598cff6adc136ba1fd606bc457e61b74cee284 Mon Sep 17 00:00:00 2001 From: Remi Gau Date: Fri, 30 Aug 2024 10:34:20 +0200 Subject: [PATCH 1/4] add test for typo --- src/schema/rules/checks/mri.yaml | 4 +- .../bidsschematools/tests/test_expressions.py | 52 ++++++++++++++++++- 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/src/schema/rules/checks/mri.yaml b/src/schema/rules/checks/mri.yaml index e2efc0216d..91a1520179 100644 --- a/src/schema/rules/checks/mri.yaml +++ b/src/schema/rules/checks/mri.yaml @@ -109,9 +109,9 @@ BolusCutOffDelayTimeNotMonotonicallyIncreasing: level: error selectors: - modality == "mri" - - sidecar.BolusCutoffDelayTime != null + - sidecar.BolusCutOffDelayTime != null checks: - - allequal(sorted(sidecar.BolusCutoffDelayTime), sidecar.BolusCutoffDelayTime) + - allequal(sorted(sidecar.BolusCutOffDelayTime), sidecar.BolusCutOffDelayTime) # 201 RepetitionTimePreparationNotConsistent: diff --git a/tools/schemacode/bidsschematools/tests/test_expressions.py b/tools/schemacode/bidsschematools/tests/test_expressions.py index 5532ef38ba..6370468453 100644 --- a/tools/schemacode/bidsschematools/tests/test_expressions.py +++ b/tools/schemacode/bidsschematools/tests/test_expressions.py @@ -1,7 +1,7 @@ import pytest from pyparsing.exceptions import ParseException -from ..expressions import ASTNode, expression +from ..expressions import Array, ASTNode, BinOp, Function, Property, expression def test_schema_expressions(schema_obj): @@ -77,3 +77,53 @@ def test_checks(schema_obj): def test_expected_failures(expr): with pytest.raises(ParseException): expression.parse_string(expr) + + +def test_valid_sidecar_field(schema_obj): + """Check sidecar fields actually exist in the metadata listed in the schema. + + Test failures are usually due to typos. + """ + for rules, level in ((schema_obj.rules.checks, 3),): + keys = (key for key in rules.keys(level=level) if key.endswith("selectors")) + check_fields(schema_obj, rules, keys) + + keys = (key for key in rules.keys(level=level) if key.endswith("checks")) + check_fields(schema_obj, rules, keys) + + +def check_fields(schema_obj, rules, keys): + for key in keys: + for rule in rules[key]: + ast = expression.parse_string(rule)[0] + if isinstance(ast, BinOp): + for half in [ast.lh, ast.rh]: + if isinstance(half, Function): + check_function(schema_obj, half) + elif isinstance(ast, Function): + check_function(schema_obj, ast) + elif isinstance(ast, Property): + check_property(schema_obj, ast) + + +def check_function(schema_obj, function): + for x in function.args: + if isinstance(x, Property): + check_property(schema_obj, x) + elif isinstance(x, Function): + check_function(schema_obj, x) + elif isinstance(x, Array): + check_array(schema_obj, x) + + +def check_array(schema_obj, array): + ( + check_property(schema_obj, element) + for element in array.elements + if isinstance(element, Property) + ) + + +def check_property(schema_obj, property): + if property.name == "sidecar": + assert property.field in schema_obj.objects.metadata From 218e8d941332bca24510c2eda75e31c3e2c928fc Mon Sep 17 00:00:00 2001 From: Remi Gau Date: Fri, 30 Aug 2024 10:49:59 +0200 Subject: [PATCH 2/4] improve --- src/schema/rules/checks/asl.yaml | 10 ++--- src/schema/rules/checks/micr.yaml | 8 ++-- .../bidsschematools/tests/test_expressions.py | 41 +++++++++++++++---- 3 files changed, 41 insertions(+), 18 deletions(-) diff --git a/src/schema/rules/checks/asl.yaml b/src/schema/rules/checks/asl.yaml index 1b42593212..2ae11ebeb0 100644 --- a/src/schema/rules/checks/asl.yaml +++ b/src/schema/rules/checks/asl.yaml @@ -196,21 +196,21 @@ ASLBackgroundSuppressionNumberPulses: - length(sidecar.BackgroundSuppressionPulseTime) == sidecar.BackgroundSuppressionNumberPulses # 181 -ASLTotalAcquiredVolumesASLContextLength: +ASLTotalAcquiredPairsASLContextLength: issue: code: TOTAL_ACQUIRED_VOLUMES_NOT_CONSISTENT message: | - The number of values for 'TotalAcquiredVolumes' for this file does not match number of + The number of values for 'TotalAcquiredPairs' for this file does not match number of volumes in the associated 'aslcontext.tsv'. - 'TotalAcquiredVolumes' is the original number of 3D volumes acquired for each volume defined in the + 'TotalAcquiredPairs' is the original number of 3D volumes acquired for each volume defined in the associated 'aslcontext.tsv'. level: warning selectors: - suffix == "asl" - type(associations.aslcontext) != "null" - - '"TotalAcquiredVolumes" in sidecar' + - '"TotalAcquiredPairs" in sidecar' checks: - - aslcontext.n_rows == sidecar.TotalAcquiredVolumes + - aslcontext.n_rows == sidecar.TotalAcquiredPairs # 184 PostLabelingDelayGreater: diff --git a/src/schema/rules/checks/micr.yaml b/src/schema/rules/checks/micr.yaml index bf2f4c55c3..7e78ab905a 100644 --- a/src/schema/rules/checks/micr.yaml +++ b/src/schema/rules/checks/micr.yaml @@ -10,17 +10,17 @@ PixelSizeInconsistent: selectors: - ome != null - sidecar.PixelSize != null - - sidecar.PixelSizeUnit != null + - sidecar.PixelSizeUnits != null checks: - | ome.PhysicalSizeX * 10 ** (-3 * index(["mm", "um", "nm"], ome.PhysicalSizeXUnit)) - == sidecar.PixelSize[0] * 10 ** (-3 * index(["mm", "um", "nm"], sidecar.PixelSizeUnit)) + == sidecar.PixelSize[0] * 10 ** (-3 * index(["mm", "um", "nm"], sidecar.PixelSizeUnits)) - | ome.PhysicalSizeY * 10 ** (-3 * index(["mm", "um", "nm"], ome.PhysicalSizeYUnit)) - == sidecar.PixelSize[1] * 10 ** (-3 * index(["mm", "um", "nm"], sidecar.PixelSizeUnit)) + == sidecar.PixelSize[1] * 10 ** (-3 * index(["mm", "um", "nm"], sidecar.PixelSizeUnits)) - | ome.PhysicalSizeZ * 10 ** (-3 * index(["mm", "um", "nm"], ome.PhysicalSizeZUnit)) - == sidecar.PixelSize[2] * 10 ** (-3 * index(["mm", "um", "nm"], sidecar.PixelSizeUnit)) + == sidecar.PixelSize[2] * 10 ** (-3 * index(["mm", "um", "nm"], sidecar.PixelSizeUnits)) # 227 InconsistentTiffExtension: diff --git a/tools/schemacode/bidsschematools/tests/test_expressions.py b/tools/schemacode/bidsschematools/tests/test_expressions.py index 6370468453..3c32cda023 100644 --- a/tools/schemacode/bidsschematools/tests/test_expressions.py +++ b/tools/schemacode/bidsschematools/tests/test_expressions.py @@ -1,7 +1,16 @@ import pytest from pyparsing.exceptions import ParseException -from ..expressions import Array, ASTNode, BinOp, Function, Property, expression +from ..expressions import ( + Array, + ASTNode, + BinOp, + Element, + Function, + Property, + RightOp, + expression, +) def test_schema_expressions(schema_obj): @@ -97,13 +106,29 @@ def check_fields(schema_obj, rules, keys): for rule in rules[key]: ast = expression.parse_string(rule)[0] if isinstance(ast, BinOp): - for half in [ast.lh, ast.rh]: - if isinstance(half, Function): - check_function(schema_obj, half) + check_binop(schema_obj, ast) elif isinstance(ast, Function): check_function(schema_obj, ast) elif isinstance(ast, Property): check_property(schema_obj, ast) + elif isinstance(ast, RightOp): + check_half(schema_obj, ast.rh) + + +def check_binop(schema_obj, binop): + for half in [binop.lh, binop.rh]: + check_half(schema_obj, half) + + +def check_half(schema_obj, half): + if isinstance(half, BinOp): + check_binop(schema_obj, half) + elif isinstance(half, Function): + check_function(schema_obj, half) + elif isinstance(half, Property): + check_property(schema_obj, half) + elif isinstance(half, Element): + check_property(schema_obj, half.name) def check_function(schema_obj, function): @@ -117,11 +142,9 @@ def check_function(schema_obj, function): def check_array(schema_obj, array): - ( - check_property(schema_obj, element) - for element in array.elements - if isinstance(element, Property) - ) + for element in array.elements: + if isinstance(element, Property): + check_property(schema_obj, element) def check_property(schema_obj, property): From cde2ba925113414d00ea7532a9c5bccd8f582eb2 Mon Sep 17 00:00:00 2001 From: Chris Markiewicz Date: Fri, 30 Aug 2024 12:08:18 -0400 Subject: [PATCH 3/4] rf: Walk all rules, collect all names --- .../bidsschematools/tests/test_expressions.py | 145 +++++++++++------- 1 file changed, 89 insertions(+), 56 deletions(-) diff --git a/tools/schemacode/bidsschematools/tests/test_expressions.py b/tools/schemacode/bidsschematools/tests/test_expressions.py index 3c32cda023..e2e0704b37 100644 --- a/tools/schemacode/bidsschematools/tests/test_expressions.py +++ b/tools/schemacode/bidsschematools/tests/test_expressions.py @@ -1,3 +1,7 @@ +from collections.abc import Mapping +from functools import singledispatch +from typing import Union + import pytest from pyparsing.exceptions import ParseException @@ -11,6 +15,7 @@ RightOp, expression, ) +from ..types import Namespace def test_schema_expressions(schema_obj): @@ -88,65 +93,93 @@ def test_expected_failures(expr): expression.parse_string(expr) +def walk_schema(schema_obj, predicate): + for key, value in schema_obj.items(): + if predicate(key, value): + yield key, value + if isinstance(value, Mapping): + for subkey, value in walk_schema(value, predicate): + yield f"{key}.{subkey}", value + + def test_valid_sidecar_field(schema_obj): """Check sidecar fields actually exist in the metadata listed in the schema. Test failures are usually due to typos. """ - for rules, level in ((schema_obj.rules.checks, 3),): - keys = (key for key in rules.keys(level=level) if key.endswith("selectors")) - check_fields(schema_obj, rules, keys) - - keys = (key for key in rules.keys(level=level) if key.endswith("checks")) - check_fields(schema_obj, rules, keys) - - -def check_fields(schema_obj, rules, keys): - for key in keys: - for rule in rules[key]: - ast = expression.parse_string(rule)[0] - if isinstance(ast, BinOp): - check_binop(schema_obj, ast) - elif isinstance(ast, Function): - check_function(schema_obj, ast) - elif isinstance(ast, Property): - check_property(schema_obj, ast) - elif isinstance(ast, RightOp): - check_half(schema_obj, ast.rh) - - -def check_binop(schema_obj, binop): - for half in [binop.lh, binop.rh]: - check_half(schema_obj, half) + field_names = {field.name for key, field in schema_obj.objects.metadata.items()} - -def check_half(schema_obj, half): - if isinstance(half, BinOp): - check_binop(schema_obj, half) - elif isinstance(half, Function): - check_function(schema_obj, half) - elif isinstance(half, Property): - check_property(schema_obj, half) - elif isinstance(half, Element): - check_property(schema_obj, half.name) - - -def check_function(schema_obj, function): - for x in function.args: - if isinstance(x, Property): - check_property(schema_obj, x) - elif isinstance(x, Function): - check_function(schema_obj, x) - elif isinstance(x, Array): - check_array(schema_obj, x) - - -def check_array(schema_obj, array): - for element in array.elements: - if isinstance(element, Property): - check_property(schema_obj, element) - - -def check_property(schema_obj, property): - if property.name == "sidecar": - assert property.field in schema_obj.objects.metadata + for key, rule in walk_schema( + schema_obj.rules, lambda k, v: isinstance(v, Mapping) and v.get("selectors") + ): + for selector in rule["selectors"]: + ast = expression.parse_string(selector)[0] + for name in find_names(ast): + if name.startswith(("json.", "sidecar.")): + assert ( + name.split(".", 1)[1] in field_names + ), f"Bad field in selector: {name} ({key})" + for selector in rule.get("checks", []): + ast = expression.parse_string(selector)[0] + for name in find_names(ast): + if name.startswith(("json.", "sidecar.")): + assert ( + name.split(".", 1)[1] in field_names + ), f"Bad field in selector: {name} ({key})" + + +def test_test_valid_sidecar_field(): + schema_obj = Namespace.build( + { + "objects": { + "metadata": { + "a": {"name": "a"}, + } + }, + "rules": {"myruleA": {"selectors": ["sidecar.a"], "checks": ["json.a == sidecar.a"]}}, + } + ) + test_valid_sidecar_field(schema_obj) + + schema_obj.objects.metadata.a["name"] = "b" + with pytest.raises(AssertionError): + test_valid_sidecar_field(schema_obj) + + +@singledispatch +def find_names(node: Union[ASTNode, str]): + # Walk AST nodes + if isinstance(node, BinOp): + yield from find_names(node.lh) + yield from find_names(node.rh) + elif isinstance(node, RightOp): + yield from find_names(node.rh) + elif isinstance(node, Array): + for element in node.elements: + yield from find_names(element) + elif isinstance(node, Element): + yield from find_names(node.name) + yield from find_names(node.index) + elif isinstance(node, (int, float)): + return + else: + raise TypeError(f"Unexpected node type: {node!r}") + + +@find_names.register +def find_function_names(node: Function): + yield node.name + for arg in node.args: + yield from find_names(arg) + + +@find_names.register +def find_property_name(node: Property): + # Properties are left-associative, so expand the left side + yield f"{next(find_names(node.name))}.{node.field}" + + +@find_names.register +def find_identifiers(node: str): + if not node.startswith(('"', "'")): + yield node From ed1c597dafb7529b4267c0f58e871f1f80ad1d20 Mon Sep 17 00:00:00 2001 From: Chris Markiewicz Date: Fri, 30 Aug 2024 12:16:55 -0400 Subject: [PATCH 4/4] Apply suggestions from code review --- tools/schemacode/bidsschematools/tests/test_expressions.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/schemacode/bidsschematools/tests/test_expressions.py b/tools/schemacode/bidsschematools/tests/test_expressions.py index e2e0704b37..33eff61000 100644 --- a/tools/schemacode/bidsschematools/tests/test_expressions.py +++ b/tools/schemacode/bidsschematools/tests/test_expressions.py @@ -119,13 +119,13 @@ def test_valid_sidecar_field(schema_obj): assert ( name.split(".", 1)[1] in field_names ), f"Bad field in selector: {name} ({key})" - for selector in rule.get("checks", []): - ast = expression.parse_string(selector)[0] + for check in rule.get("checks", []): + ast = expression.parse_string(check)[0] for name in find_names(ast): if name.startswith(("json.", "sidecar.")): assert ( name.split(".", 1)[1] in field_names - ), f"Bad field in selector: {name} ({key})" + ), f"Bad field in check: {name} ({key})" def test_test_valid_sidecar_field():