-
Notifications
You must be signed in to change notification settings - Fork 157
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] Rename "Unit" metadata to "Units" for consistency with existing fields #773
Conversation
In many cases, suffixes are restricted to one specific unit (usually |
that'd sound better (more consistent) to me. |
I am also fine with this (for all names including units). I will go ahead and update the example data sets and the validator. validator (using code suggestions): bids-standard/bids-validator@866049d (and following commits) |
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.
Full list of changed fields:
- Units
- InjectedRadioactivityUnits
- InjectedMassUnits
- SpecificRadioactivityUnits
- TracerMolecularWeightUnits
- InjectedMassPerWeightUnits
- MolarActivityUnits
- InfusionSpeedUnits
- PharmaceuticalDoseUnits
- ReconMethodParameterUnits
…cation#773 Co-authored-by: Martin Norgaard <martin.noergaard@nru.dk>
Can we get a second review? |
…cation#773 Co-authored-by: Martin Norgaard <martin.noergaard@nru.dk>
As noted in #622 (comment), we previously had in anatomy imaging data:
bids-specification/src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Lines 222 to 233 in a7faded
Given that this metadata field has the same intent, to describe the interpretation of voxel values in an imaging file, we should be consistent.
@mnoergaard @melanieganz I do not know if this means we should also change, e.g.,
InjectedMassUnit
.@tsalo I notice that we don't seem to have this field in a metadata table anywhere. Do you have a suggestion of where to add it?