-
Notifications
You must be signed in to change notification settings - Fork 160
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
[FIX] Clarify paragraph about custom data types #264
[FIX] Clarify paragraph about custom data types #264
Conversation
@bids-standard/everyone This is text being removed. I think it should be fairly non-controversial, as the text appears obviously contradictory to the rest of the document, but I want to invite people to raise any concerns if they have them. |
Can't review, but totally agree with you @effigies. |
Okay, I think this conversation ties in pretty closely to the discussion #265 about relaxing the hard "all derivatives must be in-spec" language (which I think is problematic for reasons outlined in #265 (comment), and which @satra noted is phrased very much in terms of tooling). @tyarkoni's interpretation that it is a UX choice of the validator to determine how to deal with out-of-spec files (in the case of I'm content with moving toward that language, because it seems historically supported in the text and imminently practical. As to standardizing Thoughts? |
4cf6604
to
4222205
Compare
I have pushed an update in line with @nicholst's suggestions. @bids-standard/everyone This change makes an explicit statement that I believe is consistent with the document as it stands, but runs counter to my earlier understanding. I think this one could use a lot of eyes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the present changes do the previous discussion justice and it's an improvement (clarification) to the status quo. +1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than the minor comment on scan
the PR looks good to me.
Co-Authored-By: Satrajit Ghosh <satrajit.ghosh@gmail.com>
132ffad
132ffad
Now I need you all to approve again... |
Would you care to review this PR in relation to this comment? My interpretation is that these aren't affected, as these datasets are accommodating a strict BIDS validator implementation, in accordance with the new line:
|
src/02-common-principles.md
Outdated
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All non-standard file entities SHOULD conform to BIDS conventions, including | |
All non-standard file entities SHOULD conform to BIDS-style naming conventions, including |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. ;))
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 !
Co-Authored-By: Mainak Jas <jasmainak@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Thanks for the review, everybody. |
Opened an issue in bids-standard/bids-validator#836 |
Closes #262.