-
Notifications
You must be signed in to change notification settings - Fork 161
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] allow for .png and .tif in eeg/ieeg/meg as allowed for micr #1049
Conversation
Discovered while working on bids-standard#1047
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1049 +/- ##
==========================================
+ Coverage 70.53% 71.50% +0.96%
==========================================
Files 9 9
Lines 930 930
==========================================
+ Hits 656 665 +9
+ Misses 274 265 -9 ☔ View full report in Codecov by Sentry. |
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 don't really remember why this was restricted to JPG to begin with. Probably to limit the amount of possible extensions ... as we also did with e.g., EEG file formats.
I don't think the suggestion here would hurt anyone.
Might be good to check in with all the *EG BEP leads. |
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 this makes sense, also for consistency.
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.
makes sense to me too
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.
we'd need to open an associated PR to the validator to expand the regex rules, for example here: https://github.com/bids-standard/bids-validator/blob/9c27673c4f35a2b364d09f381149dc34f6c3e12c/bids-validator/bids_validator/rules/file_level_rules.json#L380
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.
From the schema's perspective, this looks good to me. I leave it up to others to approve the content change.
Looks like lots of approval and no dissent. I agree that there doesn't seem to be much benefit in keeping this restricted, as all proposed formats are well-supported (though TIFF is more a container than a codec, I think we can assume people aren't doing really wild things with it). |
15 days since last change, 7 days since 2 approvals, no objections since then. Merging. |
Discovered while working on #1047
I guess it could be argued against but I feel that such unification makes sense. There might be other common suffixes where we might want to introduce similar unification.