Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SCHEMA] Check sidecar fields mentioned in rule checks exist in the schema #1917

Merged
merged 4 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/schema/rules/checks/asl.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
8 changes: 4 additions & 4 deletions src/schema/rules/checks/micr.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions src/schema/rules/checks/mri.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
108 changes: 107 additions & 1 deletion tools/schemacode/bidsschematools/tests/test_expressions.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,21 @@
from collections.abc import Mapping
from functools import singledispatch
from typing import Union

import pytest
from pyparsing.exceptions import ParseException

from ..expressions import ASTNode, expression
from ..expressions import (
Array,
ASTNode,
BinOp,
Element,
Function,
Property,
RightOp,
expression,
)
from ..types import Namespace


def test_schema_expressions(schema_obj):
Expand Down Expand Up @@ -77,3 +91,95 @@ def test_checks(schema_obj):
def test_expected_failures(expr):
with pytest.raises(ParseException):
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):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sort of totally improvised testing this, so there may be a simpler way to do this.

"""Check sidecar fields actually exist in the metadata listed in the schema.

Test failures are usually due to typos.
"""
field_names = {field.name for key, field in schema_obj.objects.metadata.items()}

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]
effigies marked this conversation as resolved.
Show resolved Hide resolved
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})"
effigies marked this conversation as resolved.
Show resolved Hide resolved


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
Loading