[BUG] Inconsistent level for `MagneticFieldStrength` in sidecar rules of schema

nbeliy commented 1 month ago

Describe your problem in detail.

In schema/rules/sidecars/mri.yaml, the level for MagneticFieldStrength is stated as recommended, but required for Arterial Spin Labeling, which is inconsistent with levels for other metadata fields, that only provide just level value.

Same issue with NonLinearGradientCorrection below.

Describe what you expected.

The , but required for Arterial Spin Labeling moved into level_addendum, as it is done for other metadata (for. ex EffectiveEchoSpacing)

List of possible inconsitencies

Putting all inconsistencies and possible errors that I found here:

[ ] mri/MagneticFieldStrength requirement level is not just 'optional'
[ ] mri/MRIScannerHardwareASL: suffix == "asl" contradicts intersects([suffix], ["asl", "m0scan"])
[ ] anat/TaskMetadata refers to entities list as entity.task != null, but in entity_rules, entities are queried as entities
[ ] nirs referes to sidecar as json, instead of sidecar in other rules
[ ] pet, func, dwi, anat, fmap: extension check are defined in rules/files, and not needed in sidecar
[ ] mri/MRIHardware: field HardcopyDeviceSoftwareVersion is marked as DEPRECATED, while in other similar cases (in eeg for ex.) deprecated is written in all small caps
[ ] rules for datatype perf are stored in file asl.yaml, and not as perf.yaml
[ ] Sidecar meta fields MISCChannelCount is duplicated as MiscChannelCount. First one is used i eeg, second one in meg
[ ] func: VolumeTiming : requirements for AcquisitionDuration and SliceTiming are not defined

nbeliy commented 1 month ago

Another issue with shema:

Selector for rules/sidecars/mri/MRIScannerHardwareASL is:

datatype == "perf"
suffix == "asl"
intersects([suffix], ["asl", "m0scan"])

Is it applicable only for suffix asl or for both asl and m0scan

effigies commented 1 month ago

level: <level>, <addendum> is an accidental shorthand that takes advantage of the behavior of the validator and the renderer. I'm okay with forcing consistency, though, if you would like to open a PR. If so, would you be able to also update the metaschema? I believe this line is what currently allows the above, and could be removed:

https://github.com/bids-standard/bids-specification/blob/3fd21ff095ecc9ac8e2b7bf092f5ee09e055e45b/src/metaschema.json#L668

effigies commented 1 month ago

For the MRIScannerHardwareASL, the selectors are AND-combined, so the second line should be removed, to allow it to apply to m0scans.

nbeliy commented 1 month ago

Dear, @effigies , I could do do PR, but I still feel quite unfamiliar with the structure of schema. And I'm unsure what is just historical inconsistency and what is made on purpose. For example, sometimes deprecated levels are are written in all caps, and sometimes lowercase, also at some places extensions are selected by match and in other cases, from a list.

As this schema is used to generate BIDS documentation, I'm afraid to brake something.

nbeliy commented 1 month ago

I've updated list of inconsistencies in issue.

Can you confirm that they are indeed problematic, and I will make a PR.

bids-standard / bids-specification