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

Add Siemens 2D MB PCASL example #466

Merged
merged 7 commits into from
Nov 5, 2024

Conversation

aptinis
Copy link
Contributor

@aptinis aptinis commented Aug 30, 2024

No description provided.

@aptinis
Copy link
Contributor Author

aptinis commented Aug 30, 2024

Hi there, this is the example BIDS dataset with the Siemens 2D MB Multi-delay PCASL (the same sequence used by HCP Aging) to support the PR [ENH] Add noRF and n/a ASL volume types addressing aslcontext.tsv dummy volumes. I am looking for feedback and have a couple of questions/comments.

  1. Are there any conventions to follow for naming the dataset?

  2. When I ran python tools/print_dataset_listing.py it unexpectedly modified ds001 in the dataset_listing.tsv and README.md by (incorrectly?) adding CITATION to the suffixes. In order to prevent this behaviour I added CITATION to the list of suffixes to remove. Let me know if this is okay.

  3. BIDS validation is currently failing.

There are 2 issues:

  • the BIDS validation will fail with error: TSV_VALUE_INCORRECT_TYPE (volume_type) due to the inclusion of noRF in the ASL context file (the purpose of this example is to update the BIDS spec to include this new volume type). Will this require a .SKIP_VALIDATION or .bidsignore file due to lack of coverage in the bids-validator?

  • I believe there is a bug with the BIDS validator (Schema version 0.11.0) error: JSON_SCHEMA_VALIDATION_ERROR (PostLabelingDelay) which expects PostLabelingDelay to be a number although according to the BIDS spec it can also be an array of numbers.

  1. For the noRF volumes I've set the PostLabelingDelay and LabelingDuration to 0. I have left the RepetitionTimePreparation to be the same as the tag/control volumes (although I'm not 100% sure if that's accurate).

Please let me know I've missed anything (I am new to contributing to open source projects!) or have any feedback. Thank you!

@effigies
Copy link
Contributor

 	[ERROR] JSON_SCHEMA_VALIDATION_ERROR Invalid JSON sidecar file. The sidecar is not formatted according the schema.
		PostLabelingDelay
		/sub-1/perf/sub-1_asl.json - must be number
		/sub-1/perf/sub-1_asl.json - must be > 0

The issue here is that we have an exclusive minimum:

PostLabelingDelay:
  name: PostLabelingDelay
  display_name: Post Labeling Delay
  description: |
    This is the postlabeling delay (PLD) time, in seconds, after the end of the
    labeling (for `"CASL"` or `"PCASL"`) or middle of the labeling pulse
    (for `"PASL"`) until the middle of the excitation pulse applied to the
    imaging slab (for 3D acquisition) or first slice (for 2D acquisition).
    Can be a number (for a single-PLD time series) or an array of numbers
    (for multi-PLD and Look-Locker).
    In the latter case, the array of numbers contains the PLD of each volume,
    namely each `control` and `label`, in the acquisition order.
    Any image within the time-series without a PLD, for example an `m0scan`,
    is indicated by a zero.
    Based on DICOM Tags 0018, 9079 `Inversion Times` and 0018, 0082
    `InversionTime`.
  anyOf:
    - type: number
      exclusiveMinimum: 0
      unit: s
    - type: array
      items:
        type: number
        exclusiveMinimum: 0
        unit: s

I pushed to @tsalo's branch, so will rerun these now.

@effigies

This comment was marked as outdated.

Copy link
Contributor

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Pending the upstream PR

@@ -10,7 +10,7 @@
from bids import BIDSLayout

folders_to_skip = ["docs", ".git", ".github", "tools", "env", "site", ".vscode"]
suffixes_to_remove = ["README", "description", "participants"]
suffixes_to_remove = ["README", "description", "participants", "CITATION"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching that one

Comment on lines 113 to 114
# Update this URL to the schema.json from PRs to the spec, when needed.
BIDS_SCHEMA: https://bids-specification.readthedocs.io/en/latest/schema.json
BIDS_SCHEMA: https://bids-specification--1884.org.readthedocs.build/en/1884/schema.json
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this PR but this little 'trick' to test a version of the schema related to a PR in the spec should be mentioned in our doc, so that BEP leads know how to start testing their datasets

Copy link
Contributor

Choose a reason for hiding this comment

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

#1864 was merged so this PR should be updated

Copy link
Contributor

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

good with me

@effigies effigies merged commit 38e3467 into bids-standard:master Nov 5, 2024
10 checks passed
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.

3 participants