[INFRA] Transpose the entity table and link to text anchors describing each entity by sappelhoff · Pull Request #272 · bids-standard/bids-specification

sappelhoff · 2019-07-12T14:00:22Z

closes #200

transposing done via: http://harttle.land/invert-markdown-table/
~~linking will probably work via html code~~
- ~~html code is ignored during the rendering~~
- ~~I can link to any word using this syntax: <a id="myID">this is my anchor</a>~~
- ~~... and then I need to refer to that anchor with this syntax: <a href="#myID">this is my link</a>~~
- ~~see example in pure html~~

--> linking does not work as easily as expected with the strikethrough html code I proposed above. Perhaps trying to go the "expected" way and link to headings is easier. We could make a low-in-the-hierarchy heading for each entity once it's being introduced in text.

EDIT: The situation is a bit more complicated than I anticipated. For the first time in quite a while, I read the specification consecutively, and it turns out that the key-value pairs (like sub-<label>)are never introduced explicitly or in their own section. They are fed to the reader bit by bit and it is assumed that understanding of the key-value pairs somehow "emerges".

Without a central place describing the key-value-pairs (or "entities" as we also seem to call them), it will become hard to link to specific "entities". For example, the acq parameter is mentioned several times ... and takes on a special meaning depending on the modality.

Suggestion

I think we could

add an explicit section introducing the key-value pairs (and relating them to the word "entities" and the "entity table" in the appendix)
add another section where each entity has a list of items ... and each item links to an in-text description (within the spec), where it is described what meaning that specific entity takes on in that context.

Example for point 2.

Meaning and usage of aqc
- aqc in Anatomy Imaging Data
- acq in Task Imaging Data
- acq in ...

... and in the entity table, we would link from an entity to the list of items (as shown in the example)

sappelhoff · 2019-07-12T14:15:32Z

I am not sure that the transposed format of the entity table looks better, see here:

Any opinions? cc @choldgraf @effigies @franklin-feingold

yarikoptic · 2019-07-12T14:41:26Z

FWIW
both are equivalently too big ATM, we need to figure it a way to freeze leading column/row ...
Ability to link to specific modality would be a useful addition IMHO. Also I like that in transposed version all key-value pairs are listed horizontally, as if they appeared in a file name - easier to relate.

sappelhoff · 2019-07-12T14:46:35Z

we need to figure it a way to freeze leading column/row ...

I absolutely agree ... I spent some minutes trying to find something: No luck so far.

But I take your comment as a vote that

transposing
links from entities table to their first in-text reference

would be an improvement.

sappelhoff · 2019-07-12T17:25:44Z

@yarikoptic it seems like we'd have to write custom CSS to get the sticky first row and column, see: squidfunk/mkdocs-material#1171

yarikoptic · 2019-07-12T18:00:26Z

Thanks! Sticky header seems indeed simple https://codepen.io/tfzvang/pen/WQBwVo

With transposition it will make it easy to align desired to of the file to the key-value pairs

choldgraf · 2019-07-12T18:56:24Z

seems good to me!

franklin-feingold · 2019-07-13T00:47:54Z

Thank you @sappelhoff !

+1 the transposed is looking good and as Yarik said I think it makes it easier to relate and map onto the filenames. Linking from the modality in the entity table to where it is described in the text should help connect the two. Linking to the modality will bring the user to the section they are interested in, perhaps not needing to go into each key (e.g. acq)

As you mentioned Stefan - we don't explicitly define the key-value pairs. The closest is in the context of describing the json file format (https://bids-specification.readthedocs.io/en/latest/02-common-principles.html#keyvalue-files-dictionaries). The common principles section would be the best location. Perhaps right around the section I linked?

What do we think?

sappelhoff · 2019-07-25T13:47:04Z

I agree with @franklin-feingold and I have prepared a draft with a short intro.

A new round of reviews would be appreciated. I think this is an acceptable improvement

yarikoptic

minor comment on sub-label

but I wonder -- so there is no any kind of ids for the modalities in the table -- it doesn't work at all?

src/02-common-principles.md

effigies

Some consistency and language comments. Additionally, we need to modify the table CSS, because this is very difficult to read with the word wraps in the format strings.

Additionally, column headers are expected to apply to columns, not rows, so we may want to reconsider what's in the top left.

Another thought I had is that we may want to make this into a couple tables, possible an MR/PET and an Electrophys table, and drop entities that don't sensibly apply (such as dir- for ephys). I think this might improve readability a lot.

src/02-common-principles.md

effigies · 2019-07-25T14:14:32Z

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

-
-<sup>4</sup>[http://fsl.fmrib.ox.ac.uk/fsl/fsl4.0/fdt/fdt_dtifit.html](http://fsl.fmrib.ox.ac.uk/fsl/fsl4.0/fdt/fdt_dtifit.html)
+are in the [FSL format](http://fsl.fmrib.ox.ac.uk/fsl/fsl4.0/fdt/fdt_dtifit.html):
+The bvec files contain 3 rows with n space-delimited floating-point numbers


As an aside, this is a good example of why I suggested that we add newlines after sentences, as I now need to read every line to verify that the only change is in the link.

I'll open an issue to restart discussion there.

Yes, that is a good point, perhaps we can even enforce this with our linter 🤔

see https://github.com/why-jay/remark-lint-sentence-newline @effigies

Cool, thanks. I have a draft I'm working on, but this is surprisingly hard to write in a way that makes it sound like a useful suggestion rather than an annoying one...

src/02-common-principles.md

effigies · 2019-07-25T18:09:55Z

src/02-common-principles.md


+## File names consist of key-value pairs (entities)
+
+We define file names with a chain of key-value pairs, each of which we call


The "We" and "you" that show up a bit here feel off relative to most of the document. I think we generally use the passive voice or imperative mood. If it would be useful to have a native English speaker do a sweep through to uniformize the voice, I can do that.

That'd be great 👍

See sappelhoff#2.

effigies · 2019-07-25T18:14:32Z

src/99-appendices/04-entity-table.md

+| meg<br>                                                                                        | REQUIRED      | OPTIONAL      | REQUIRED       | OPTIONAL      |                          |                |                          | OPTIONAL      |                        |                |                     | OPTIONAL              |                 |
+| eeg<br>                                                                                        | REQUIRED      | OPTIONAL      | REQUIRED       | OPTIONAL      |                          |                |                          | OPTIONAL      |                        |                |                     |                       |                 |
+| ieeg<br>                                                                                       | REQUIRED      | OPTIONAL      | REQUIRED       | OPTIONAL      |                          |                |                          | OPTIONAL      |                        |                |                     |                       |                 |
+| channels<br>(meg/eeg/ieeg)                                                                     | REQUIRED      | OPTIONAL      | REQUIRED       |               |                          |                |                          | OPTIONAL      |                        |                |                     |                       |                 |


From here down, the format has switched from DATATYPE (SUFFIX1 SUFFIX2 SUFFIX3) to SUFFIX (DATATYPE1 ...). I recognize that it was there before, so whether we want to fix it here or push it to another PR, I'm okay either way).

there are more extensive changes to be done (also table Css, potentially splitting per modality, ...), so let's keep the PRs short and focused - that keeps motivation up and improvements flowing in regularly

effigies · 2019-07-25T18:20:01Z

minor comment on sub-label

but I wonder -- so there is no any kind of ids for the modalities in the table -- it doesn't work at all?

@yarikoptic Could you reword this? I don't understand what you're trying to say.

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

effigies

Two very small cleanups, and then I'm good with this.

src/99-appendices/04-entity-table.md

src/02-common-principles.md

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

sappelhoff · 2019-07-30T08:13:59Z

Two very small cleanups, and then I'm good with this.

cool thanks.

Then once this is merged, I'll open a new issue to track the next enhancements:

Make the first row and first column "sticky" using custom CSS code
Potentially separate the entity table into several sub-tables (one per modality)
1. this would also make it easier to insert links from an entity in the table to an in-text description of what that entity means in the context of a certain modality
Address some inconsistencies in labelling in the entity table, see [INFRA] Transpose the entity table and link to text anchors describing each entity #272 (comment) for reference

I would keep all of these separate issues, because that makes them more attractive to tackle. WDYT?

Finally, @effigies could you open an issue related to "each sentence on its own line" (see: #272 (comment)), please?

sappelhoff · 2019-08-01T06:50:26Z

I think we can merge this. @yarikoptic's concerns will be addressed in a future PR (I'll open an issue upon merge of this PR). @franklin-feingold ... perhaps you want to give it one more review pass?

franklin-feingold

thank you @sappelhoff ! Looks good!

I think this is resolved.

effigies · 2019-08-01T19:50:38Z

@yarikoptic Any remaining qualms?

* origin/master: [DOC] Auto-generate changelog entry for PR bids-standard#272 Apply suggestions from code review reorder sections as chris suggested STY: Move to passive voice/imperative mood consistent use of <label> Apply suggestions from code review right align cells and formtting add general section on entities fix linter some cleanup/clarifying try links via tiny headings try fix links try links transpose entity table

sappelhoff added the opinions wanted Please read and offer your opinion on this matter label Jul 12, 2019

sappelhoff added this to the 1.2.1 milestone Jul 22, 2019

sappelhoff added 6 commits July 25, 2019 14:44

transpose entity table

4874e64

try links

bea27b0

try fix links

2e1cccd

try links via tiny headings

5d9c33f

some cleanup/clarifying

e6f8a59

fix linter

059e4b1

sappelhoff force-pushed the entitylink branch from f186d3e to 059e4b1 Compare July 25, 2019 13:00

add general section on entities

857ed07

sappelhoff marked this pull request as ready for review July 25, 2019 13:32

sappelhoff requested a review from chrisgorgo as a code owner July 25, 2019 13:32

right align cells and formtting

5bcde18

sappelhoff requested review from effigies, franklin-feingold and yarikoptic July 25, 2019 17:01

yarikoptic previously requested changes Jul 25, 2019

View reviewed changes

src/02-common-principles.md Outdated Show resolved Hide resolved

effigies reviewed Jul 25, 2019

View reviewed changes

sappelhoff and others added 3 commits July 25, 2019 20:54

Apply suggestions from code review

8e2d9e8

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

consistent use of <label>

472f0fa

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

STY: Move to passive voice/imperative mood

a6a4356

reorder sections as chris suggested

dbfe38a

effigies reviewed Jul 29, 2019

View reviewed changes

src/99-appendices/04-entity-table.md Outdated Show resolved Hide resolved

src/02-common-principles.md Outdated Show resolved Hide resolved

Apply suggestions from code review

dc351fa

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

effigies approved these changes Jul 30, 2019

View reviewed changes

franklin-feingold approved these changes Aug 1, 2019

View reviewed changes

sappelhoff mentioned this pull request Aug 1, 2019

Entity Table (Appendix) improvements: Custom CSS #289

Open

franklin-feingold mentioned this pull request Aug 2, 2019

REL: v1.2.1 #291

Merged

4 tasks

sappelhoff merged commit cfa313c into bids-standard:master Aug 5, 2019

sappelhoff deleted the entitylink branch August 5, 2019 19:44

franklin-feingold added a commit that referenced this pull request Aug 5, 2019

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

85eb60e


		## File names consist of key-value pairs (entities)

		We define file names with a chain of key-value pairs, each of which we call

Conversation

sappelhoff commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Suggestion

Uh oh!

sappelhoff commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yarikoptic commented Jul 12, 2019

Uh oh!

sappelhoff commented Jul 12, 2019

Uh oh!

sappelhoff commented Jul 12, 2019

Uh oh!

yarikoptic commented Jul 12, 2019

Uh oh!

choldgraf commented Jul 12, 2019

Uh oh!

franklin-feingold commented Jul 13, 2019

Uh oh!

sappelhoff commented Jul 25, 2019

Uh oh!

yarikoptic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effigies commented Jul 25, 2019

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sappelhoff commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sappelhoff commented Aug 1, 2019

Uh oh!

franklin-feingold left a comment

Choose a reason for hiding this comment

Uh oh!

effigies commented Aug 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sappelhoff commented Jul 12, 2019 •

edited

Loading

sappelhoff commented Jul 12, 2019 •

edited

Loading

sappelhoff commented Jul 30, 2019 •

edited

Loading