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.

Sign up for GitHub

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

Jump to bottom

[FIX] Clarify paragraph about custom data types #264

Merged
merged 5 commits into from
Sep 18, 2019
Merged

[FIX] Clarify paragraph about custom data types #264

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4222205
[SPEC] Remove misleading paragraph about custom data types
effigies Jul 3, 2019
b19c1d7
ENH: Update guidance for non-standard files
effigies Sep 13, 2019
48ecc27
Update src/02-common-principles.md
effigies Sep 14, 2019
132ffad
Update src/02-common-principles.md
effigies Sep 17, 2019
d875e95
Update src/02-common-principles.md
effigies Sep 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions src/02-common-principles.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,6 @@ sub-control01/
sub-control01_phasediff.json
sub-control01_magnitude1.nii.gz
sub-control01_scans.tsv

code/
deface.py
derivatives/
Expand All @@ -440,8 +439,20 @@ dataset_description.json
CHANGES
```

effigies marked this conversation as resolved.
Show resolved Hide resolved
Additional files and folders containing raw data may be added as needed for
special cases. They should be named using all lowercase with a name that
effigies marked this conversation as resolved.
Show resolved Hide resolved
reflects the nature of the scan (e.g., `calibration`). Naming of files within
the directory should follow the same scheme as above (e.g.,
`sub-control01_calibration_Xcalibration.nii.gz`)
## Unspecified data

Additional files and folders containing raw data MAY be added as needed for
special cases.
All non-standard file entities SHOULD conform to BIDS conventions, including
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
All non-standard file entities SHOULD conform to BIDS conventions, including
All non-standard file entities SHOULD conform to BIDS-style naming conventions, including

Copy link
Collaborator

Choose a reason for hiding this comment

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

This by the way means that anything you put in .bidsignore SHOULD also follow BIDS-style naming convention, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would think so, yes.

Copy link
Collaborator

@jasmainak jasmainak Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

okay, so let's raise an issue on bids-validator to follow up on it before merging this PR? According to this rephrasing, any file including those in .bidsignore should be of format:

key1-value1_key2-value2_..._suffix.ext

This should be easy enough to implement. I predict that at least some example datasets will fail the validator once you implement this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

SHOULD is not MUST - this is more of a recommendation than a mandate.

Copy link

Choose a reason for hiding this comment

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

I also don't see the point of constraining the format of .bidsignore. Suppose I have a subdir called misc/ that contains a bunch of files that are relevant to the project, some of which are BIDS-like, some of which aren't, but none of which I want to run the validator (or some other app) against. Why would I need/want to put anything other than misc/ in .bidsignore? And if I can't specify anything other than BIDS-like patterns, that means I don't have a way of telling validators or other apps to ignore non-compliant paths I know are going to fail validation or raise warnings, which seems to defeat the point of having the .bidsignore in the first place.

Re: SHOULD vs. MAY, there isn't a fact of the matter; it's always going to be a judgment call whether someone thinks they have a good reason to ignore the recommendation. It's also going to be a judgment call at implementation: one validator may choose to raise a warning for violation of SHOULDs whereas another may not. It's true that in practice, some people are going to do the work to make non-compliant files BIDS-like, while others won't make the effort. But that's exactly the point of a non-binding recommendation: we would like you to do it, but if you don't, so be it. (To be clear, I agree that we SHOULD raise warnings in the validator for this; I just don't think it's a MUST. ;))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, perhaps I was a little too flippant; adding it to the validator does provide a useful hint to curators, so there is a value. My point was more that, no tools can be expected to parse everything that might show up in a .bidsignore, so the only real thing to consider is "Will a human understand this", which is outside anything a validator can hope for.

But yes, I agree that it would be a good addition to the validator, but I think there's more value in having the language clear in the spec sooner than waiting for somebody to fix the validator giving a friendly prod. (If you'd like to submit a PR, I'd be happy to review it, although my Javascript is rusty.)

Copy link
Collaborator

@jasmainak jasmainak Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Why would I need/want to put anything other than misc/ in .bidsignore?

Yes you are right. You should be able to put misc/ in .bidsignore but I still think the validator should raise a warning for each of the files in misc/ that are not BIDS-like.

adding it to the validator does provide a useful hint to curators, so there is a value.

yes exactly, and historically we have raised warnings in the validator for SHOULD and RECOMMENDED stuff (even though not completely in a consistent way).

I think there's more value in having the language clear in the spec sooner than waiting for somebody to fix the validator giving a friendly prod.

To be clear, I'm all for clarification of the language and I don't want to stop that.

If you'd like to submit a PR, I'd be happy to review it, although my Javascript is rusty

I'd rather have someone else submit the PR so we do not have a community of people making changes to the specification faster than the tooling can keep up. I'm happy to review the PR though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jasmainak Just to be clear, I see this as a clarification of the present document that does not change the intended meaning, and requires no changes to any datasets or tooling. I agree with the move to propose that the validator should specifically handle this case and warn on ignored-but-unBIDSy files.

So I would suggest we go ahead and merge this and open an issue on bids-validator. Does that work for you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay sounds fair to me !

alphabetic entities and suffixes and alphanumeric labels/indices.
Non-standard suffixes SHOULD reflect the nature of the data, and existing
entities SHOULD be used when appropriate.
For example, an ASSET calibration scan might be named
`sub-01_acq-ASSET_calibration.nii.gz`.

Non-standard files and directories should be named with care.
Future BIDS efforts may standardize new entities and suffixes, changing the
meaning of file names and setting requirements on their contents or metadata.
Validation and parsing tools MAY treat the presence of non-standard files and
directories as an error, so consult the details of these tools for mechanisms
to suppress warnings or provide interpretations of your file names.