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

[enh][PET] add example for ReconFilterType is 'none' #427

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bendhouseart
Copy link

Adds an example dataset to help with the closing of this issue -> bids-standard/bids-validator#1897 on the BIDS Validator

@bendhouseart bendhouseart marked this pull request as ready for review February 26, 2024 16:09
pet006/README Outdated Show resolved Hide resolved
effigies added a commit to bids-standard/bids-validator that referenced this pull request Mar 3, 2024
@effigies
Copy link
Contributor

effigies commented Mar 7, 2024

Should be fixed in bids-validator@master. Bumping CI.

Comment on lines +24 to +26
"ReconMethodParameterLabels": [
"none"
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, reading the spec, it says

REQUIRED if ReconMethodParameterLabels does not contain "none"

but that seems odd. If it's ["abc", "def", "none"], presumably you want units and values. I suppose the text should actually say 'is not ["none"]'? I can probably do that in schema. Matching any might require me to bug Ross.

What is currently implemented is:

Suggested change
"ReconMethodParameterLabels": [
"none"
],
"ReconMethodParameterLabels": "none",

We could update the text to allow that, but I think probably ["none"] is the cleaner solution, so that the type is constant.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mnoergaard Would you mind commenting on the original intent here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The original intent goes back to the PET guidelines paper (https://journals.sagepub.com/doi/10.1177/0271678X20905433) where the ReconMethodParameterLabels was decided to be mandatory. In most cases, there are multiple reconstruction parameters and labels, however, for some older scanners there no recon parameters. As far as I remember, for the old validator, it was not easy to combine datatypes depending on the need, so it was decided to keep this as an array of strings. However, it is not so clean in the case of no parameters (even with ["none"]. Just "none" would be the cleanest.

@effigies effigies force-pushed the add-recon-filter-type-none-example branch from e598827 to 676a60f Compare May 9, 2024 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants