[FIX][SCHEMA] Add conditionals for PET ReconMethod* and ReconFilter

[bendhouseart]

Adds conditionals to change required PET fields to optional when "parent" fields are unknown or "none", specifically:

• add conditionals for ReconMethod Metadata

• add min and max values to scatter fraction

• add conditional for ReconFilterSize

Intention is to avoid users adding unnecessary place holders to pass validation, e.g. [0, 0], ["none", "none"], hopefully avoids ambiguity instead of increasing it.

Arguments could be made that fields should always be required, but several recent use cases have prompted this minor change to the spec. Additionally, it's more straight forward (to this user) to toggle optional than to define null placeholders for values and datatypes.

[codecov]

Codecov Report

Base: 88.39% // Head: 88.33% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (d616e82) compared to base (2383c56).
Patch has no changes to coverable lines.

❗ Current head d616e82 differs from pull request most recent head 3341a07. Consider uploading reports for the commit 3341a07 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1299      +/-   ##
==========================================
- Coverage   88.39%   88.33%   -0.07%     
==========================================
  Files          11       11              
  Lines        1086     1080       -6     
==========================================
- Hits          960      954       -6     
  Misses        126      126              

Impacted Files	Coverage Δ
tools/schemacode/bidsschematools/render/text.py	97.39% <0.00%> (-0.08%)	⬇️
...ools/schemacode/bidsschematools/render/__init__.py	100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[tsalo]

I think the rules need to be reconfigured a bit.

[effigies]

@bendhouseart While working through some examples, we've found that it's going to be very difficult to both render and enforce rules that have required fields and then relax under certain conditions. So we can do two things:

1. Make the optional default and use level_addendum to describe when it's required, and then add a required rule with a selector. Here only the first table will get rendered, and it will say something like OPTIONAL, but REQUIRED if XYZ.

PETReconstruction:
  selectors:
    - modality == "pet"
    - suffix == "pet"
  fields:
    ReconMethodParameterLabels:
      level: required
    ReconMethodParameterUnits:
      level: optional
      level_addendum: |
        required if `ReconMethodParameterLabels` does not contain 'none'

PETReconstructionReq:
  selectors:
    - modality == "pet"
    - suffix == "pet"
    - '!intersects(sidecar.ReconMethodParameterLabels, ["none"])'
  fields:
    ReconMethodParameterUnits: required

2. Split the rule into two separate tables, one required and one optional, and then combine during the render:

PETReconstruction:
  selectors:
    - modality == "pet"
    - suffix == "pet"
  fields:
    ReconMethodParameterLabels:
      level: required

PETReconstructionReq:
  selectors:
    - modality == "pet"
    - suffix == "pet"
    - '!intersects(sidecar.ReconMethodParameterLabels, ["none"])'
  fields:
    ReconMethodParameterUnits:
      level: required
      level_addendum: |
        optional if `ReconMethodParameterLabels` does not contain 'none'

PETReconstructionOpt:
  selectors:
    - modality == "pet"
    - suffix == "pet"
    - intersects(sidecar.ReconMethodParameterLabels, ["none"])
    ReconMethodParameterUnits: optional

{{ MACROS___make_sidecar_table(["pet.PETReconstruction". "pet.PETReconstructionReq"]) }}

[CPernet]

@effigies isn't OPTIONAL but REQUIRED if similar to what is already used for 'NonlinearGradientCorrection' which then would make sense to use as a rule for conditional stuff -- although that case this is RECOMMENDED, but REQUIRED if (which would also actually be better for ReconMethod Parameters and ReconFilter Parameters

[effigies]

Yes, these are equivalent approaches in terms of enforcement. The choice between the two is which way PET maintainers would prefer it to be rendered.

Note that if you make a field RECOMMENDED, there will be warnings when it is missing. (The current validator does not follow this rule, but the schematized one will.)

[melanieganz]

@effigies, so we just discussed this again in the OpenNeuroDev meeting and we agreed upon going for RECOMMENDED, but REQUIRED if . We'd like it to spit out a warning during validation , since we want to encourage people to go look for the values, but we accept that they might be hard to find/dig out.

[effigies]

Looks right. Some styling fixes for the table.

[effigies]

Final render:

[effigies]

@melanieganz @CPernet @mnoergaard Any of you up for giving this another review and a green check if you're happy with it?

Changes applied

[sappelhoff]

tagging again: @melanieganz @CPernet @mnoergaard

[CPernet]

yep that's what we discussed, if not then recommended (to avoid validation error) - thx @effigies

[sappelhoff]

Thanks all!