Skip to content

[MISC] Expand entity table for MEG/EEG/iEEG specific files#198

Merged
sappelhoff merged 5 commits intobids-standard:masterfrom
sappelhoff:enh_entities_appendix
Apr 25, 2019
Merged

[MISC] Expand entity table for MEG/EEG/iEEG specific files#198
sappelhoff merged 5 commits intobids-standard:masterfrom
sappelhoff:enh_entities_appendix

Conversation

@sappelhoff
Copy link
Copy Markdown
Member

@sappelhoff sappelhoff commented Apr 6, 2019 •
edited
Loading

closes #173

labelled misc because it's not really a new feature but was simply forgotten when merging EEG and iEEG

A human readable version of the changes is here: https://520-150465237-gh.circle-artifacts.com/0/home/circleci/project/site/99-appendices/04-entity-table.html (NOTE: this link is outdated, check the fresh circleCI build to see / ask me how to do that)

or probably easier, see the google doc (NOTE: gdoc is now outdated)

@sappelhoff sappelhoff added the misc label Apr 6, 2019
@choldgraf
Copy link
Copy Markdown
Collaborator

choldgraf commented Apr 7, 2019

Two quick things (that are technically out of scope for this PR, so feel free to say "let's do this in another PR").

  • I think the table should be transposed. The columns will continue growing while the rows probably won't, and already we have a wider-than-higher table, which I think would render better if it were transposed. This is pretty easy to do with a little pandas, happy to try helping out here.
  • Could we add lists to the bottom of this table that include the list of extensions associated with each type of file? I think that'd make the table a bit easier to parse since currently some cells are a lot bigger than others.

The changes directly in this PR look good to me though!

@sappelhoff
Copy link
Copy Markdown
Member Author

sappelhoff commented Apr 7, 2019

Those sound like to excellent suggestions to me! A little help would be greatly appreciated: Ideally in the form of a new PR, as you suggested :-)

@choldgraf choldgraf mentioned this pull request Apr 7, 2019
Closed
@choldgraf
Copy link
Copy Markdown
Collaborator

choldgraf commented Apr 7, 2019

done #200

choldgraf
choldgraf previously approved these changes Apr 7, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM - a few changes I think we should make in this table (but not related to this PR, issue opened in #200 )

@sappelhoff
Copy link
Copy Markdown
Member Author

sappelhoff commented Apr 12, 2019

@robertoostenveld @dorahermes would you have time to review this in the next week? Or can you suggest someone to review in your stead?

@jasmainak
Copy link
Copy Markdown
Collaborator

jasmainak commented Apr 20, 2019

It's really hard to know looking at the rendering what was added and what wasn't :-(

To answer your question from the google doc, proc is only for sss or tsss data AFAIK and that too it is not clear because BIDS is not supposed to cover derived data for MEG at the moment

@monkeyman192
Copy link
Copy Markdown
Collaborator

monkeyman192 commented Apr 23, 2019

It's really hard to know looking at the rendering what was added and what wasn't :-(

I agree and I would say the table itself is getting a bit hard to read since it is so large, but I can't really think of a better way to format it...

@sappelhoff
Copy link
Copy Markdown
Member Author

sappelhoff commented Apr 23, 2019

Yep, the bad readability will be taken care of in a different PR, Chris H has already opened an issue: #200

Unless there are complaints soon, I will go ahead and merge this ... and then we can improve it even further in future PRs. At least we will have EEG and iEEG modalities/files represented.

robertoostenveld
robertoostenveld requested changes Apr 23, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@robertoostenveld robertoostenveld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a deja vu when recent emails on this PR came in, but now realize that this was also discussed here.

The label task should not be required for eeg/ieeg electrodes (it is just like headshape and photo). It should also not be optional. Looking at the published spec for ieeg and eeg, I see that ieeg does not have it, but eeg has. I see that meg also does not have it.

So task-<label> for electrodes should not only be removed here, but also in the EEG spec.

@sappelhoff sappelhoff dismissed stale reviews from franklin-feingold and choldgraf via 245bf27 April 23, 2019 09:43
@sappelhoff
Copy link
Copy Markdown
Member Author

sappelhoff commented Apr 23, 2019

Thanks @robertoostenveld I removed the task parameter for electrodes. In the spec, it will be removed in #188, which is also ready to be merged from my perspective

choldgraf
choldgraf previously approved these changes Apr 24, 2019
View reviewed changes
robertoostenveld
robertoostenveld previously approved these changes Apr 25, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@robertoostenveld robertoostenveld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acq-<label> will be removed from meg, eeg, ieeg as per ##188. That should also be reflected here. Consequently, acq-<label> should also not be optional for meg/eeg/ieeg events.

Other than that I an fine, so let me approve already without requiring another pass at it.

@sappelhoff sappelhoff dismissed stale reviews from robertoostenveld and choldgraf via fb5ab37 April 25, 2019 10:08
@sappelhoff
Copy link
Copy Markdown
Member Author

sappelhoff commented Apr 25, 2019

Thanks for the reviews. I am merging this now and with #200 we will further improve on this.

@sappelhoff sappelhoff force-pushed the enh_entities_appendix branch 3 times, most recently from 5a3d5f2 to fe6f594 Compare April 25, 2019 10:32
@sappelhoff sappelhoff merged commit 9e02f3d into bids-standard:master Apr 25, 2019
@sappelhoff sappelhoff deleted the enh_entities_appendix branch April 25, 2019 10:50
@sappelhoff sappelhoff mentioned this pull request Feb 18, 2020
Closed
@jasmainak jasmainak mentioned this pull request Mar 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robertoostenveld robertoostenveld robertoostenveld left review comments

@choldgraf choldgraf choldgraf left review comments

@dorahermes dorahermes Awaiting requested review from dorahermes

@jasmainak jasmainak Awaiting requested review from jasmainak

@monkeyman192 monkeyman192 Awaiting requested review from monkeyman192

@franklin-feingold franklin-feingold Awaiting requested review from franklin-feingold

Assignees

No one assigned

Labels

misc

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

EEG/iEEG suffixes are not listed in the Entities appendix

6 participants

@sappelhoff @choldgraf @jasmainak @monkeyman192 @robertoostenveld @franklin-feingold