From 3a636a1f11bc2549ee97dd7d7d2e1813d21814a8 Mon Sep 17 00:00:00 2001 From: Chris Markiewicz Date: Thu, 18 Jul 2024 17:28:46 -0400 Subject: [PATCH 1/4] fix(checks): Sort columns.onset numerically (#1871) * enh(expressions): Allow numeric or lexical sorting * fix(checks): Sort columns.onset numerically * enh(expressions): Test behavior of numerically sorting n/a --- src/schema/README.md | 2 +- src/schema/meta/expression_tests.yaml | 10 ++++++++++ src/schema/rules/checks/events.yaml | 2 +- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/schema/README.md b/src/schema/README.md index 14290337c1..68be4f2a9b 100644 --- a/src/schema/README.md +++ b/src/schema/README.md @@ -270,7 +270,7 @@ The following functions should be defined by an interpreter: | `match(arg: str, pattern: str) -> bool` | `true` if `arg` matches the regular expression `pattern` (anywhere in string) | `match(extension, ".gz$")` | True if the file extension ends with `.gz` | | `max(arg: array) -> number` | The largest non-`n/a` value in an array | `max(columns.onset)` | The time of the last onset in an events.tsv file | | `min(arg: array) -> number` | The smallest non-`n/a` value in an array | `min(sidecar.SliceTiming) == 0` | A check that the onset of the first slice is 0s | -| `sorted(arg: array) -> array` | The sorted values of the input array | `sorted(sidecar.VolumeTiming) == sidecar.VolumeTiming` | True if `sidecar.VolumeTiming` is sorted | +| `sorted(arg: array, method: str) -> array` | The sorted values of the input array; defaults to type-determined sort. If method is "lexical", or "numeric" use lexical or numeric sort. | `sorted(sidecar.VolumeTiming) == sidecar.VolumeTiming` | True if `sidecar.VolumeTiming` is sorted | | `substr(arg: str, start: int, end: int) -> str` | The portion of the input string spanning from start position to end position | `substr(path, 0, length(path) - 3)` | `path` with the last three characters dropped | | `type(arg: Any) -> str` | The name of the type, including `"array"`, `"object"`, `"null"` | `type(datatypes)` | Returns `"array"` | diff --git a/src/schema/meta/expression_tests.yaml b/src/schema/meta/expression_tests.yaml index 8ab60a28d4..0d5d54a1f2 100644 --- a/src/schema/meta/expression_tests.yaml +++ b/src/schema/meta/expression_tests.yaml @@ -110,6 +110,16 @@ result: null - expression: sorted([3, 2, 1]) result: [1, 2, 3] +- expression: sorted([1, 2, 5, 10], "lexical") + result: [1, 10, 2, 5] +- expression: sorted(["1", "2", "5", "10"]) + result: ["1", "10", "2", "5"] +- expression: sorted(["1", "2", "5", "10"], "numeric") + result: ["1", "2", "5", "10"] +- expression: sorted(["1", "2", "n/a"], "numeric") + result: ["1", "2", "n/a"] +- expression: sorted(["n/a", "2", "1"], "numeric") + result: ["n/a", "1", "2"] - expression: allequal(sorted([3, 2, 1]), [1, 2, 3]) result: true # Regression test. Javascript will sort lexically by default. diff --git a/src/schema/rules/checks/events.yaml b/src/schema/rules/checks/events.yaml index 234b9d078d..1f0f57a123 100644 --- a/src/schema/rules/checks/events.yaml +++ b/src/schema/rules/checks/events.yaml @@ -39,4 +39,4 @@ SortedOnsets: - extension == ".tsv" checks: # n/a values will likely cause false alarms if encountered. Consider alternatives. - - allequal(sorted(columns.onset), columns.onset) + - allequal(sorted(columns.onset, "numeric"), columns.onset) From c0917c337413df39cf73db73f120d1db9b7a6e83 Mon Sep 17 00:00:00 2001 From: Chris Markiewicz Date: Thu, 18 Jul 2024 17:31:51 -0400 Subject: [PATCH 2/4] fix(schema): metaschema cleanup, schema bugs caught (#1870) * chore(metaschema): Organize enums and termTypes in definitions * fix(metaschema): Improve definitions of requirement levels, fields and issues * fix(schema): Inconsistencies caught by improved metaschema * rf(metaschema): Define and use expressionList * fix(metaschema): Remove unused README property * rf(metaschema): Move single-use definition back inline * fix(metaschema): Unify derivative and raw file rules --- src/metaschema.json | 332 ++++++++++++---------------- src/schema/rules/sidecars/func.yaml | 2 +- src/schema/rules/sidecars/mri.yaml | 13 +- 3 files changed, 154 insertions(+), 193 deletions(-) diff --git a/src/metaschema.json b/src/metaschema.json index dd1a3fcb91..ce500d6418 100644 --- a/src/metaschema.json +++ b/src/metaschema.json @@ -12,10 +12,7 @@ "type": "object", "properties": { "selectors": { - "type": "array", - "items": { - "type": "string" - } + "$ref": "#/definitions/ruleTypes/expressionList" }, "target": { "type": "object", @@ -85,13 +82,13 @@ "patternProperties": { "^[a-zA-Z0-9_]+$": { "allOf": [ - { "$ref": "#/definitions/schemaTerm" }, - { "$ref": "#/definitions/generalTerm" }, - { "$ref": "#/definitions/nameValueTerm" }, + { "$ref": "#/definitions/termTypes/JSONSchema" }, + { "$ref": "#/definitions/termTypes/general" }, + { "$ref": "#/definitions/termTypes/nameValue" }, { "type": "object", "properties": { - "format": { "$ref": "#/definitions/formats" }, + "format": { "$ref": "#/definitions/enums/formats" }, "unit": { "type": "string" } } } @@ -105,7 +102,7 @@ "type": "object", "patternProperties": { "^[a-zA-Z0-9_]+$": { - "$ref": "#/definitions/generalTerm", + "$ref": "#/definitions/termTypes/general", "unevaluatedProperties": false }, "additionalProperties": false @@ -116,8 +113,8 @@ "patternProperties": { "^[a-zA-Z0-9_]+$": { "allOf": [ - { "$ref": "#/definitions/generalTerm" }, - { "$ref": "#/definitions/valueTerm" } + { "$ref": "#/definitions/termTypes/general" }, + { "$ref": "#/definitions/termTypes/value" } ], "unevaluatedProperties": false } @@ -129,13 +126,13 @@ "patternProperties": { "^[a-zA-Z0-9_]+$": { "allOf": [ - { "$ref": "#/definitions/schemaTerm" }, - { "$ref": "#/definitions/generalTerm" }, - { "$ref": "#/definitions/nameValueTerm" }, + { "$ref": "#/definitions/termTypes/JSONSchema" }, + { "$ref": "#/definitions/termTypes/general" }, + { "$ref": "#/definitions/termTypes/nameValue" }, { "type": "object", "properties": { - "format": { "$ref": "#/definitions/formats" } + "format": { "$ref": "#/definitions/enums/formats" } }, "required": ["format"] } @@ -150,8 +147,8 @@ "patternProperties": { "^[a-zA-Z0-9][a-zA-Z0-9_-]*$": { "allOf": [ - { "$ref": "#/definitions/generalTerm" }, - { "$ref": "#/definitions/valueTerm" }, + { "$ref": "#/definitions/termTypes/general" }, + { "$ref": "#/definitions/termTypes/value" }, { "properties": { "tags": { "type": "array" } } } ] }, @@ -168,8 +165,8 @@ "patternProperties": { "^[a-zA-Z0-9_]+$": { "allOf": [ - { "$ref": "#/definitions/generalTerm" }, - { "$ref": "#/definitions/valueTerm" } + { "$ref": "#/definitions/termTypes/general" }, + { "$ref": "#/definitions/termTypes/value" } ], "unevaluatedProperties": false } @@ -181,7 +178,7 @@ "patternProperties": { "^[a-zA-Z0-9_]+$": { "allOf": [ - { "$ref": "#/definitions/generalTerm" }, + { "$ref": "#/definitions/termTypes/general" }, { "properties": { "file_type": { "type": "string" } }, "required": ["file_type"] @@ -194,10 +191,10 @@ }, "formats": { "type": "object", - "propertyNames": { "$ref": "#/definitions/formats" }, + "propertyNames": { "$ref": "#/definitions/enums/formats" }, "additionalProperties": { "allOf": [ - { "$ref": "#/definitions/generalTerm" }, + { "$ref": "#/definitions/termTypes/general" }, { "type": "object", "properties": { @@ -216,9 +213,9 @@ "patternProperties": { "^[a-zA-Z0-9_]+$": { "allOf": [ - { "$ref": "#/definitions/schemaTerm" }, - { "$ref": "#/definitions/generalTerm" }, - { "$ref": "#/definitions/nameValueTerm" }, + { "$ref": "#/definitions/termTypes/JSONSchema" }, + { "$ref": "#/definitions/termTypes/general" }, + { "$ref": "#/definitions/termTypes/nameValue" }, { "type": "object", "properties": { @@ -238,7 +235,7 @@ "type": "object", "patternProperties": { "^[a-z]+$": { - "$ref": "#/definitions/generalTerm", + "$ref": "#/definitions/termTypes/general", "unevaluatedProperties": false } }, @@ -249,8 +246,8 @@ "patternProperties": { "^[a-zA-Z0-9_]+$": { "allOf": [ - { "$ref": "#/definitions/generalTerm" }, - { "$ref": "#/definitions/valueTerm" }, + { "$ref": "#/definitions/termTypes/general" }, + { "$ref": "#/definitions/termTypes/value" }, { "type": "object", "properties": { @@ -296,26 +293,19 @@ "^[a-zA-Z0-9_]+$": { "type": "object", "properties": { - "issues": { - "type": "object", - "properties": { - "code": { "type": "string" }, - "message": { "type": "string" }, - "level": { "enum": ["error", "warning"] } - }, - "required": ["code", "message", "level"], - "additionalProperties": false + "issue": { + "allOf": [{ "$ref": "#/definitions/ruleTypes/issue" }], + "required": ["level"] }, "selectors": { - "type": "array", - "items": { "type": "string" } + "$ref": "#/definitions/ruleTypes/expressionList" }, "checks": { - "type": "array", - "items": { "type": "string" } + "$ref": "#/definitions/ruleTypes/expressionList" } }, - "required": ["checks", "selectors"] + "required": ["checks", "selectors"], + "additionalProperties": false } } } @@ -359,18 +349,10 @@ "deriv": { "type": "object", "patternProperties": { - "^[a-zA-Z0-9_]+$": { + "^[a-z_]+$": { "type": "object", "patternProperties": { - "^[a-zA-Z0-9_]+$": { - "type": "object", - "properties": { - "entities": { - "$ref": "#/definitions/entities" - } - } - }, - "additionalProperties": false + "^[a-zA-Z0-9_]+$": { "$ref": "#/definitions/suffixRule" } } } }, @@ -454,10 +436,7 @@ "message": { "type": "string" }, "level": { "enum": ["error", "warning"] }, "selectors": { - "type": "array", - "items": { - "type": "string" - } + "$ref": "#/definitions/ruleTypes/expressionList" } }, "required": ["message", "level"], @@ -501,81 +480,100 @@ }, "schema_version": { "type": "string" - }, - "README": { - "type": "string" } }, "required": ["meta", "objects", "rules", "bids_version", "schema_version"], "additionalProperties": false, "definitions": { - "formats": { - "$comment": "Formats whose patterns are defined in BIDS schema", - "type": "string", - "enum": [ - "index", - "label", - "boolean", - "integer", - "number", - "string", - "hed_version", - "bids_uri", - "dataset_relative", - "date", - "datetime", - "file_relative", - "participant_relative", - "rrid", - "stimuli_relative", - "time", - "unit", - "uri" - ] - }, - "generalTerm": { - "type": "object", - "properties": { - "display_name": { "type": "string" }, - "description": { "type": "string" } + "enums": { + "formats": { + "$comment": "Formats whose patterns are defined in BIDS schema", + "type": "string", + "enum": [ + "index", + "label", + "boolean", + "integer", + "number", + "string", + "hed_version", + "bids_uri", + "dataset_relative", + "date", + "datetime", + "file_relative", + "participant_relative", + "rrid", + "stimuli_relative", + "time", + "unit", + "uri" + ] }, - "required": ["display_name", "description"] - }, - "schemaTerm": { - "$ref": "https://json-schema.org/draft/2020-12/schema#", - "anyOf": [{ "required": ["type"] }, { "required": ["anyOf"] }] - }, - "nameValueTerm": { - "type": "object", - "properties": { "name": { "type": "string" } }, - "required": ["name"] + "requirement_level": { + "$comment": "Requirement levels that may apply to terms", + "type": "string", + "enum": ["required", "recommended", "optional", "deprecated"] + } }, - "valueTerm": { - "type": "object", - "properties": { "value": { "type": "string" } }, - "required": ["value"] + "termTypes": { + "general": { + "type": "object", + "properties": { + "display_name": { "type": "string" }, + "description": { "type": "string" } + }, + "required": ["display_name", "description"] + }, + "JSONSchema": { + "$ref": "https://json-schema.org/draft/2020-12/schema#", + "anyOf": [{ "required": ["type"] }, { "required": ["anyOf"] }] + }, + "nameValue": { + "type": "object", + "properties": { "name": { "type": "string" } }, + "required": ["name"] + }, + "value": { + "type": "object", + "properties": { "value": { "type": "string" } }, + "required": ["value"] + } }, - "entities": { - "type": "object", - "patternProperties": { - "^[a-z]+$": { - "anyOf": [ - { "enum": ["optional", "required"] }, - { - "type": "object", - "properties": { - "level": { "enum": ["optional", "required"] }, - "enum": { - "type": "array", - "items": { "type": "string" } - } - }, - "required": ["level", "enum"] - } - ] - } + "ruleTypes": { + "field": { + "anyOf": [ + { "$ref": "#/definitions/enums/requirement_level" }, + { + "type": "object", + "properties": { + "level": { "$ref": "#/definitions/enums/requirement_level" }, + "level_addendum": { "type": "string" }, + "description_addendum": { "type": "string" }, + "issue": { "$ref": "#/definitions/ruleTypes/issue" } + }, + "required": ["level"], + "additionalProperties": false + }, + { "type": "string", "pattern": "recommended.*" } + ] }, - "additionalProperties": false + "issue": { + "type": "object", + "properties": { + "code": { "type": "string" }, + "message": { "type": "string" }, + "level": { "enum": ["error", "warning"] } + }, + "required": ["code", "message"], + "additionalProperties": false + }, + "expressionList": { + "type": "array", + "items": { + "type": "string" + } + } }, "sidecar": { "type": "object", @@ -583,41 +581,12 @@ "^[a-zA-Z0-9_]+$": { "type": "object", "properties": { - "selectors": { - "type": "array", - "items": { "type": "string" } - }, + "selectors": { "$ref": "#/definitions/ruleTypes/expressionList" }, "fields": { "type": "object", "patternProperties": { "^[a-zA-Z0-9_]+$": { - "anyOf": [ - { - "enum": [ - "recommended", - "optional", - "required", - "deprecated" - ] - }, - { - "type": "object", - "properties": { - "level": { - "enum": [ - "recommended", - "optional", - "required", - "deprecated" - ] - }, - "level_addendum": { "type": "string" } - }, - "required": ["level", "level_addendum"], - "additionalProperties": false - }, - { "pattern": "recommended.*" } - ] + "$ref": "#/definitions/ruleTypes/field" } }, "additionalProperties": false @@ -635,42 +604,12 @@ "^[a-zA-Z0-9_]+$": { "type": "object", "properties": { - "selectors": { - "type": "array", - "items": { "type": "string" } - }, + "selectors": { "$ref": "#/definitions/ruleTypes/expressionList" }, "columns": { "type": "object", "patternProperties": { "^[a-zA-Z0-9_]+$": { - "anyOf": [ - { - "enum": [ - "recommended", - "optional", - "required", - "deprecated" - ] - }, - { - "type": "object", - "properties": { - "level": { - "enum": [ - "recommended", - "optional", - "required", - "deprecated" - ] - }, - "level_addendum": { "type": "string" }, - "description_addendum": { "type": "string" } - }, - "required": ["level"], - "additionalProperties": false - }, - { "pattern": "recommended.*" } - ] + "$ref": "#/definitions/ruleTypes/field" } }, "additionalProperties": false @@ -725,7 +664,28 @@ "items": { "pattern": "^[a-zA-Z0-9]+$" } }, "extensions": { "type": "array", "items": { "type": "string" } }, - "entities": { "$ref": "#/definitions/entities" } + "entities": { + "type": "object", + "patternProperties": { + "^[a-z]+$": { + "anyOf": [ + { "enum": ["optional", "required"] }, + { + "type": "object", + "properties": { + "level": { "enum": ["optional", "required"] }, + "enum": { + "type": "array", + "items": { "type": "string" } + } + }, + "required": ["level", "enum"] + } + ] + } + }, + "additionalProperties": false + } }, "required": ["suffixes", "extensions", "entities"], "additionalProperties": false diff --git a/src/schema/rules/sidecars/func.yaml b/src/schema/rules/sidecars/func.yaml index f3b7e70b69..dbb4d714e0 100644 --- a/src/schema/rules/sidecars/func.yaml +++ b/src/schema/rules/sidecars/func.yaml @@ -58,7 +58,7 @@ MRIFuncTimingParameters: field and that do not have the `SliceTiming` field set to allow for accurate calculation of "acquisition time" issue: - name: VOLUME_TIMING_MISSING_ACQUISITION_DURATION + code: VOLUME_TIMING_MISSING_ACQUISITION_DURATION message: | The field 'VolumeTiming' requires 'AcquisitionDuration' or 'SliceTiming' to be defined. DelayAfterTrigger: recommended diff --git a/src/schema/rules/sidecars/mri.yaml b/src/schema/rules/sidecars/mri.yaml index cc27d676be..c9b94ec83b 100644 --- a/src/schema/rules/sidecars/mri.yaml +++ b/src/schema/rules/sidecars/mri.yaml @@ -27,7 +27,8 @@ MRIHardware: description_addendum: Corresponds to DICOM Tag 0018, 1020 `Software Versions`. HardcopyDeviceSoftwareVersion: deprecated MagneticFieldStrength: - level: recommended, but required for Arterial Spin Labeling + level: recommended + level_addendum: required for Arterial Spin Labeling ReceiveCoilName: recommended ReceiveCoilActiveElements: recommended NumberReceiveCoilActiveElements: optional @@ -170,14 +171,14 @@ PhaseEncodingDirectionReq: PhaseEncodingDirection: level: required issue: - name: PHASE_ENCODING_DIRECTION_MUST_DEFINE - issue: | + code: PHASE_ENCODING_DIRECTION_MUST_DEFINE + message: | You have to define 'PhaseEncodingDirection' for this file. TotalReadoutTime: level: required description_addendum: 3 issue: - name: TOTAL_READOUT_TIME_MUST_DEFINE + code: TOTAL_READOUT_TIME_MUST_DEFINE message: | You have to define 'TotalReadoutTime' for this file. @@ -191,7 +192,7 @@ MRITimingParameters: required if corresponding fieldmap data is present, or the data comes from a multi-echo sequence or Arterial Spin Labeling. issue: - name: ECHO_TIME_NOT_DEFINED + code: ECHO_TIME_NOT_DEFINED message: | You must define 'EchoTime' for this file. 'EchoTime' is the echo time (TE) for the acquisition, specified in seconds. Corresponds to DICOM Tag @@ -273,7 +274,7 @@ MRIFlipAngleLookLockerTrue: FlipAngle: level: required issue: - name: LOOK_LOCKER_FLIP_ANGLE_MISSING + code: LOOK_LOCKER_FLIP_ANGLE_MISSING message: | You should define 'FlipAngle' for this file, in case of a LookLocker acquisition. 'FlipAngle' is the From 65ca8ddc6d897246c1f820edb3b135662e1bda2d Mon Sep 17 00:00:00 2001 From: Chris Markiewicz Date: Thu, 18 Jul 2024 17:34:55 -0400 Subject: [PATCH 3/4] fix(schema): Deduplicate enum values constructed from anyOfs (#1872) --- tools/schemacode/bidsschematools/schema.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tools/schemacode/bidsschematools/schema.py b/tools/schemacode/bidsschematools/schema.py index ed6b5da5f3..a00eaddac9 100644 --- a/tools/schemacode/bidsschematools/schema.py +++ b/tools/schemacode/bidsschematools/schema.py @@ -164,7 +164,10 @@ def flatten_enums(namespace, inplace=True): namespace = deepcopy(namespace) for struct in _find(namespace, lambda obj: "anyOf" in obj): try: - all_enum = [val for item in struct["anyOf"] for val in item["enum"]] + # Deduplicate because JSON schema validators may not like duplicates + # Long run, we should get rid of this function and have the rendering + # code handle anyOfs + all_enum = list(dict.fromkeys(val for item in struct["anyOf"] for val in item["enum"])) except KeyError: continue From 5cac7c5a9c19869c546af3ed585ad7c1e9402f4b Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 22 Jul 2024 21:09:49 +0000 Subject: [PATCH 4/4] [pre-commit.ci] pre-commit autoupdate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit updates: - [github.com/pre-commit/mirrors-mypy: v1.10.1 → v1.11.0](https://github.com/pre-commit/mirrors-mypy/compare/v1.10.1...v1.11.0) --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 886c93bce6..c59c7ce96a 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -67,7 +67,7 @@ repos: - id: codespell args: ["--config=.codespellrc", "--dictionary=-", "--dictionary=.codespell_dict"] - repo: https://github.com/pre-commit/mirrors-mypy - rev: v1.10.1 + rev: v1.11.0 hooks: - id: mypy # Sync with project.optional-dependencies.typing