[FIX] Clarify data types and requirement levels for all JSON files #605

sappelhoff · 2020-09-14T08:13:34Z

closes #350
closes #221
closes #533
fixes part of bids-standard/bids-validator#1065

this is a draft to get comments on whether this would be something we want for the entire spec.

Also some specific questions:

Does somebody have a better link to describe JSON strings than https://www.w3schools.com/js/js_json_syntax.asp ?
What do you think of the "data type" column entry for Levels --> [object][object] with string: string key-value pairs is that clear enough? --> in JSON all keys MUST be strings ... but I wanted to make the point that the values in the Levels object MUST be strings as well.

TO DO

render pdf in landscape so that tables are easier to read
decrease the extremely large margins in the PDF build (currently 1.5 inch), so that tables are displayed more easily --> perhaps go to 0.75 inch
- ~~center~~ align the "fancyheader" and "fancyfooter" (see header.tex) to make the PDF look nice with smaller marging
make sure that no tables break in the pdf build (see e.g., https://github.com/bids-standard/bids-specification/pull/605/files#r488467181) --> NOTE: Some single rows do break, e.g., AnatomicalLandmarkCoordinateSystemDescription ... simply because the "key" is too long and will overlap with the req. level column. that happens only a few times and is unavoidable at this point.
double check that all tables render in the html build

PDF status

~~I don't know what's happening ... in a8cb249 everything is nice ... one commit later the tables are broken again: 357bfd3 (--> checkout the circle ci build artifact in each commit)~~

--> fixed. This was a funny mix of python's os.walk not being deterministic, a broken MD table, and a python-table-rendering script that did not warn upon error :)

tsalo

I think the level of detail is quite reasonable.

tsalo · 2020-09-14T15:22:18Z

@sappelhoff do you want to convert the PR to a draft to make sure no one accidentally merges it once reviews are in?

effigies · 2020-09-14T16:02:42Z

This looks reasonable.

I don't know of a better reference for strings.
We could say [object][] of [strings][string], as examples should make clear what that means, but being explicit about key/value pairs is also fine.

src/05-derivatives/03-imaging.md

pdf_build_src/header_setup.tex

src/04-modality-specific-files/03-electroencephalography.md

effigies · 2020-09-16T14:09:19Z

Reviewing, but as an initial comment, the Requirement Level column takes up a ton of space for 2 bits of information. What if we added abbreviations to use? Some options and evaluations:

REQUIRED	RECOMMENDED	OPTIONAL	Thoughts
REQ	REC	OPT	Straightforward, REQ and REC look and are pronounced similar
RQRD	RCMD	OPTL	Less confusable. Ugly.
Req	Rec	Opt	No longer CAPS, but they are used in isolation, q looks unlike c
MUST	SHLD	MAY	Shorter, look like official alternatives, semantically weird
MUST	SHOULD	MAY	Same as above, SHOULD looks less weird though

If we do go with an abbreviation, I would specify it in the common principles, and I would change the header from Requirement Level to simply Level (also to be described in common principles).

src/04-modality-specific-files/08-genetic-descriptor.md

src/05-derivatives/02-common-data-types.md

src/05-derivatives/03-imaging.md

src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md

effigies · 2020-09-16T14:47:51Z

src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md

+| ---------------------- | ---------------------------------------------------------------------------------------------------------------- | ------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| EchoTime               | RECOMMENDED, but REQUIRED if corresponding fieldmap data is present or the data comes from a multi echo sequence | [number][]               | The echo time (TE) for the acquisition, specified in seconds. Corresponds to DICOM Tag 0018, 0081 `Echo Time` (please note that the DICOM term is in milliseconds not seconds).                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              |
+| InversionTime          | RECOMMENDED                                                                                                      | [number][]               | The inversion time (TI) for the acquisition, specified in seconds. Inversion time is the time after the middle of inverting RF pulse to middle of excitation pulse to detect the amount of longitudinal magnetization. Corresponds to DICOM Tag 0018, 0082 `Inversion Time` (please note that the DICOM term is in milliseconds not seconds).                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                |
+| SliceTiming            | RECOMMENDED, but REQUIRED for sparse sequences that do not have the `DelayTime` field set                        | [array][] of [numbers][] | The time at which each slice was acquired within each volume (frame) of the acquisition. Slice timing is not slice order -- rather, it is a list of times containing the time (in seconds) of each slice acquisition in relation to the beginning of volume acquisition. The list goes through the slices along the slice axis in the slice encoding dimension (see below). Note that to ensure the proper interpretation of the `SliceTiming` field, it is important to check if the OPTIONAL `SliceEncodingDirection` exists. In particular, if `SliceEncodingDirection` is negative, the entries in `SliceTiming` are defined in reverse order with respect to the slice axis (i.e., the final entry in the `SliceTiming` list is the time of acquisition of slice 0). Without this parameter slice time correction will not be possible. |


i think it might make the most sense to keep the requirement level brief, e.g. RECOMMENDED/REQUIRED, and add the REQUIRED for sparse sequences that do not have the DelayTime field set to the end of the description.

src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md

also make os.walk behave deterministically

Co-authored-by: Chris Markiewicz <[email protected]>

sappelhoff · 2020-09-17T08:28:41Z

Thanks @effigies, I addressed all your comments except the following:

shortening the "Requirement Level" column

Reviewing, but as an initial comment, the Requirement Level column takes up a ton of space for 2 bits of information. What if we added abbreviations to use?

link to review comment

I see your point, but I find the new layout aesthetically pleasing, and have not too big of a motivation to decrease the width of the Requirement column manually.

Perhaps this could be something to return to once we start generating these tables automatically?

Need some more opinions @tsalo @yarikoptic

Keeping the "Requirement level" column brief

i think it might make the most sense to keep the requirement level brief, e.g. RECOMMENDED/REQUIRED, and add the REQUIRED for sparse sequences that do not have the DelayTime field set to the end of the description.

link to review comment

Personally, I find it nicer to read everything about requirements in the requirement level column. It makes more sense to me logically, but you do have a point that a very wide "requirement level" column looks ugly ... especially when there are few rows that need the width, and most rows could actually get along with having a very short width.

Yet in rendered form, this looks quite okay and nice IMHO:

So I'd be in favor of keeping it as it is

src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md

effigies

Looks good. One small suggestion.

src/04-modality-specific-files/04-intracranial-electroencephalography.md

effigies · 2020-09-17T13:42:10Z

Oh, and FWIW I agree that the tables look fine in HTML. In the PDF, the description is pushed quite far to the right.

…ography.md Co-authored-by: Chris Markiewicz <[email protected]>

sappelhoff · 2020-09-19T10:36:58Z

@tsalo could you please have a final look again? I'd like to get this merged as soon as possible to avoid merge conflicts ... this PR is touching almost every table in the spec after all :-)

tsalo

I noticed one minor issue but other than that everything looks great to me.

src/04-modality-specific-files/02-magnetoencephalography.md

sappelhoff · 2020-09-19T19:57:39Z

Thanks for the reviews, this is going in now.

sappelhoff added the opinions wanted Please read and offer your opinion on this matter label Sep 14, 2020

sappelhoff requested review from yarikoptic, effigies, robertoostenveld and tsalo September 14, 2020 08:13

tsalo approved these changes Sep 14, 2020

View reviewed changes

sappelhoff marked this pull request as draft September 14, 2020 16:03

sappelhoff added this to the 1.4.1 milestone Sep 14, 2020

sappelhoff force-pushed the datatypes branch from 8428508 to 301547f Compare September 15, 2020 07:44

sappelhoff commented Sep 15, 2020

View reviewed changes

src/05-derivatives/03-imaging.md Show resolved Hide resolved

sappelhoff commented Sep 15, 2020

View reviewed changes

pdf_build_src/header_setup.tex Outdated Show resolved Hide resolved

sappelhoff requested a review from sebastientourbier September 15, 2020 08:15

sappelhoff mentioned this pull request Sep 15, 2020

GeneticLevel in schema expects array of str, but should ALSO accept a string bids-standard/bids-validator#1065

Closed

sappelhoff added 12 commits September 15, 2020 19:30

datatype draft

4d4dcf4

try smaller margins pdf for better tables

1b9ca3d

specify paper size and margins via tex header

d08fe4d

back to default margins, but valid specification now

609e6b9

make use of [link][] notation for brevity

fa17f72

enh tables in modality agnostic files

74c1e02

enh tables in common data types

d420970

work on 03-imaging

a0a747d

enh genetics tables with req level and datatype

3ed71ff

enh physio+stim tables

b34e9d6

enh ieeg, concistency ephys

9999631

fmt fixes, clarify geneticlevel bids-standard/bids-validator#1065

cd1def4

sappelhoff force-pushed the datatypes branch from 79e006c to cd1def4 Compare September 15, 2020 17:31

enh tables in EEG section

5a44332

effigies reviewed Sep 15, 2020

View reviewed changes

src/04-modality-specific-files/03-electroencephalography.md Outdated Show resolved Hide resolved

sappelhoff added 3 commits September 16, 2020 15:34

decrease margins, drop a4 paper in favor of default

2a36ce6

center header and footer disregarding odd and even pages

1b3afaa

add links, try pdf landscape

a8cb249

effigies reviewed Sep 16, 2020

View reviewed changes

sappelhoff added 2 commits September 16, 2020 17:15

adjust cover abd header for landscape pdf

357bfd3

fix: skip broken tables during python script

0d8cc76

also make os.walk behave deterministically

This was referenced Sep 16, 2020

Change "e.g." in the spec to "for example" #608

Closed

Json files for EEG, MEG, iEEG can't be "augmented" like those for BOLD? bids-standard/bids-validator#1044

Closed

sappelhoff and others added 2 commits September 17, 2020 10:02

add missing datatypes for mri

13fdd9a

misc fixes

053950e

Co-authored-by: Chris Markiewicz <[email protected]>

add missing link

f16cfbc

effigies reviewed Sep 17, 2020

View reviewed changes

src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md Show resolved Hide resolved

effigies approved these changes Sep 17, 2020

View reviewed changes

src/04-modality-specific-files/04-intracranial-electroencephalography.md Outdated Show resolved Hide resolved

sappelhoff and others added 2 commits September 17, 2020 16:52

Update src/04-modality-specific-files/04-intracranial-electroencephal…

5914cc7

…ography.md Co-authored-by: Chris Markiewicz <[email protected]>

fix pipe

e8fe54b

This was referenced Sep 17, 2020

Add a note on what "deprecation" means in the spec #611

Closed

Add MRI schemas bold.json etc. bids-standard/bids-validator#1074

Open

tsalo requested changes Sep 19, 2020

View reviewed changes

src/04-modality-specific-files/02-magnetoencephalography.md Show resolved Hide resolved

add missing link

e87497c

tsalo approved these changes Sep 19, 2020

View reviewed changes

sappelhoff merged commit 8ae43ce into bids-standard:master Sep 19, 2020

sappelhoff deleted the datatypes branch September 19, 2020 19:57

bids-maintenance added a commit that referenced this pull request Sep 19, 2020

[DOC] Auto-generate changelog entry for PR #605

c0e8ca7

This was referenced Feb 12, 2021

validate correct data type (value) of some fields such as MagneticFieldStrength bids-standard/bids-validator#851

Closed

RepetitionTime type in bold.json not specified. #516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Clarify data types and requirement levels for all JSON files #605

[FIX] Clarify data types and requirement levels for all JSON files #605

sappelhoff commented Sep 14, 2020 •

edited

Loading

tsalo left a comment

tsalo commented Sep 14, 2020

effigies commented Sep 14, 2020

effigies commented Sep 16, 2020

effigies Sep 16, 2020

sappelhoff commented Sep 17, 2020

effigies left a comment

effigies commented Sep 17, 2020

sappelhoff commented Sep 19, 2020

tsalo left a comment

sappelhoff commented Sep 19, 2020

[FIX] Clarify data types and requirement levels for all JSON files #605

[FIX] Clarify data types and requirement levels for all JSON files #605

Conversation

sappelhoff commented Sep 14, 2020 • edited Loading

TO DO

PDF status

tsalo left a comment

Choose a reason for hiding this comment

tsalo commented Sep 14, 2020

effigies commented Sep 14, 2020

effigies commented Sep 16, 2020

effigies Sep 16, 2020

Choose a reason for hiding this comment

sappelhoff commented Sep 17, 2020

shortening the "Requirement Level" column

Keeping the "Requirement level" column brief

effigies left a comment

Choose a reason for hiding this comment

effigies commented Sep 17, 2020

sappelhoff commented Sep 19, 2020

tsalo left a comment

Choose a reason for hiding this comment

sappelhoff commented Sep 19, 2020

sappelhoff commented Sep 14, 2020 •

edited

Loading