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] BEP022 - Magnetic Resonance Spectroscopy #425

Merged
merged 41 commits into from
Aug 29, 2024

Conversation

markmikkelsen
Copy link
Contributor

This pull request adds MRS BIDS-compliant datasets to the bids-examples repository. It includes three datasets: mrs_2dmrsi/, mrs_biggaba/, and mrs_fmrs/.

@markmikkelsen markmikkelsen changed the title [] Bep022 [ENH] BEP022 - Magnetic Resonance Spectroscopy Feb 7, 2024
@Remi-Gau
Copy link
Contributor

@markmikkelsen
do you think you could add a README.md to each dataset?

one of them has an excellent README but the 2 others have none

There is a README template in the bids starter kit if that can help: https://github.com/bids-standard/bids-starter-kit/blob/main/templates/README.MD?plain=1

@Remi-Gau
Copy link
Contributor

@effigies do you think we should / could use this PR to use citation.cff files?

I think we do not have any example that make use of them.

@effigies
Copy link
Contributor

effigies commented Feb 13, 2024

Not unless Mark already has CITATION.cff files he wants to include. We don't need to pile on tasks to this BEP. Let's make a separate PR that adds a citation file to an existing example.

@Remi-Gau
Copy link
Contributor

oh yeah of course!! that's fair!

will open an issue to see if we can actually do it in another dataset that already exist.

@Remi-Gau
Copy link
Contributor

🤦🏾 👴🏾

@markmikkelsen
Copy link
Contributor Author

@markmikkelsen do you think you could add a README.md to each dataset?

one of them has an excellent README but the 2 others have none

There is a README template in the bids starter kit if that can help: https://github.com/bids-standard/bids-starter-kit/blob/main/templates/README.MD?plain=1

Yep! I've just added these files.

@markmikkelsen
Copy link
Contributor Author

markmikkelsen commented Feb 27, 2024

This pull request is nearly ready for merging. The check failures relate to the MRS filename suffices not being included in the bids-validator.

dataset_listing.tsv Outdated Show resolved Hide resolved
- Change suffix from `ref` to `mrsref` where applicable
- Amend dataset_listing.tsv and README.md
dataset_listing.tsv Outdated Show resolved Hide resolved
@effigies
Copy link
Contributor

There are no examples of nuc- or voi- entities. These would be helpful for validating entity ordering.

@markmikkelsen
Copy link
Contributor Author

There are no examples of nuc- or voi- entities. These would be helpful for validating entity ordering.

I'm ignorant of how the validator works, but does it rely on examples? My impression was that it was contingent on the current stable specification. So, how do example datasets factor into validation?

@Remi-Gau
Copy link
Contributor

There are no examples of nuc- or voi- entities. These would be helpful for validating entity ordering.

I'm ignorant of how the validator works, but does it rely on examples? My impression was that it was contingent on the current stable specification. So, how do example datasets factor into validation?

in short and in a very hand wavy way:

the validator tries to implement the specification so we need examples to ensure that the validator work as expected (they act as some sort of ground truth)

so the richer the examples, the more we can be sure that the validator has fewer blind spots

@markmikkelsen
Copy link
Contributor Author

Gotcha. Since the datasets are not real. I can just create a bunch of example of fake datasets to satisfy this.

effigies and others added 5 commits July 30, 2024 15:05
for MRSI in mrs_{2dmrsi,biggaba}/sub-*/mrs/*.json; do
    jq '.SpectrometerFrequency = [.SpectrometerFrequency] | .ResonantNucleus = [.ResonantNucleus]' $MRSI \
    | sponge $MRSI;
done
npx prettier -w mrs_2dmrsi/sub-*/mrs/*.json
npx prettier --tab-width 4 -w mrs_biggabba/sub-*/mrs/*.json
@Remi-Gau Remi-Gau added the BEP label Aug 27, 2024
@effigies
Copy link
Contributor

@markmikkelsen @wtclarke Note that BIDS doesn't permit null values, but instead leaves fields blank. Fixed in bc92a4d. Not sure if you need to update converters.

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.

@Remi-Gau Would you mind a final review, since I've been pushing to this?

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.

could not spot anything

@effigies effigies merged commit f03d69a into bids-standard:master Aug 29, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants