-
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
[FIX] Recommend SI units formatting to adhere to CMIXF-12 #411
Conversation
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.
The table looks good. I have some suggested alternative wording for the new text. Hopefully this is more unambiguous about what is permitted and what is required.
I dropped the bit about multiple unicode code points. If you feel it's worth working back in, that's fine.
Thanks @effigies, I think your wording was more unambiguous and I added it. I think we could still clarify a bit more with the units, before we try to get this PR in. 1. For encoding a unit, can a user pick any combination of:
... or do we require e.g. SYMBOL and UNIT 2. (implied in 1.) Can a user write out the complete unit? E.g., 3. Regarding lower and upper case, I assume that we go with how it's handled in CMIXF-12, yet it may be good to make this more obvious with an additional sentence. |
b24a70d
to
c22ecbc
Compare
let me just make a proposal that's easier to criticize than my 3 questions above. Let's add this sentence right before the table:
feel free to add an example. Or adjust this text to be more clear. |
should we clarify that combinations of units are allowed if required. For example slew rate ( |
@sappelhoff I agree in principle with what you are proposing, but we should also note that CMIXF-12 does have conventions for derived units, including parentheses, exponents, multiplication and division. |
reminder: this PR will also need to be coordinated with #391 |
This is a biggish change, I'd like to get some more feedback on this @yarikoptic @robertoostenveld @nicholst @oesteban |
I think this issue deserves more attention, so let me write a summary for everbody to catch up and offer their opinion. Selective tag list (please tag more people that I forgot): @yarikoptic @robertoostenveld @nicholst @oesteban @franklin-feingold @emdupre @Remi-Gau @satra @VisLab @dorahermes @effigies This issue is about how to format units in BIDS. Brief history
The problemI think it's great that the validator now checks units ... but this also put the finger on the wound in our spec: Namely, that the way units are currently defined in BIDS is unclear with regards to formatting. We clearly state that we RECOMMEND SI units but we are unclear as to how we want these units to be formatted. Some users have taken a look at our units appendix and deduced that they can make use of any combination of:
For example: µV, µVolt, microVolt, microV as formattings of "micro volts" and this is also exactly what the validator currently checks. Yet nowhere in the spec to we REQUIRE this behavior. So other users have (arguably reasonably) formatted their SI units in some other ways. E.g., uV to reflect "micro volts". Current situation and where to go from hereThis brings us to the current situation: The specification is unclear with how to format units, but the validator is strict in what it accepts as a valid unit. This PR is intended to clarify how we want to have units formatted in BIDS. If you care about this problem, please offer your opinion ... and if you know somebody who might care, please refer them to this place :-) 🚀 |
I like the clarity of MIXF but I can understand that doesn't totally specify things. This seems like an area where others have put some thought, though... I don't have time to digest, but it seems the following will be helpful...
The last one, an IEEE/ANSI standard, even has a section titled "Application of SI prefixes". I'm hoping we can simply direct readers to one of these standards to say "do it that way, but render it as per MIXF". |
This comment has been minimized.
This comment has been minimized.
@nicholst - can you unpack this a bit more? there is an entire yacc parser for cmixf-12 at the end of the document, so technically it does provide a complete representation of formatting as well. also i know there are some additional issues in BIDS w.r.t units that we have to tackle such as the fact that certain things (age, height, weight) represented as SI are likely to impose significant constraints on certain groups in certain countries. (perhaps this should be a separate issue, but likely to come up in all contexts). |
Hi @satra, I should have rephrased my comment as "I interpret @sappelhoff's message to mean that MIXF doesn't totally specify things" :) @sappelhoff: Maybe you can clarify what's missing from MIXF? |
I don't think that there is something missing from MIXF. And I do think that it may be a nice way forward to require MIXF formatting of SI units. Yet, I see several issues that need to be discussed with more people than the usual suspects, because we have a high chance of breaking existing tools and datasets:
I agree with @nicholst that it would be nice to simply refer to another standard: "Please use units according to this" ... and even better if there is an existing validator for that standard that we can simply tie into our @satra yes I think that it'd be helpful to split off the discussion about age, and I am interested to hear more about why you think weight and height could be problematic to be represented in SI. |
Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Mainak Jas <[email protected]>
Co-authored-by: Thomas Nichols <[email protected]>
I went through the PR once more and changed the REQUIRED keywords back to RECOMMENDED. Now the PR achieves the following:
From what I see, we are not breaking any previous versions of BIDS. Only making some recommendations where previously we were unclear. This PR is now ready to be merged from my side. Please review and approve ... or tell me that I overlooked something. And thanks for your patience 👍 |
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.
LGTM
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.
Most suggestions are to use [<section name>](...)
rather than [<section name> section](...)
, which I think is more consistent with how we link elsewhere in the document. We may be somewhat inconsistent, as well, so feel free to reject if you think this damages readability.
Note that there are a number of links ending parentheticals, so the parentheticals aren't getting closed.
A couple other comments, but otherwise this LGTM.
src/04-modality-specific-files/04-intracranial-electroencephalography.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Chris Markiewicz <[email protected]>
Sorry, yes. The "Review" button did not show that I had these in queue, I guess because they were outdated. I'll delete them. |
I will merge this PR early next week, so everybody has some more time to complain or approve ... a bit like they say at some weddings:
|
Thanks a lot to everybody who helped along the way. Especially to @satra who brought this up. |
replaces #151 as proposed by @satra , please see discussion therein!
Here is a summary:
This PR is intended to explicitly state how we want the FORMATTING of SI units to be. Namely, we prefer ASCII formatting according to MIXF (metric interchange format), see --> http://people.csail.mit.edu/jaffer/MIXF/CMIXF-12.
For backward compatibility, we will allow unicode characters, but further clarify the exact unicode we expect (e.g, U+00B5 for the micro sign µ).
Please see the proposed changes in this PR and approve or discuss!
more recent summary: #411 (comment)
To do