Skip to content
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

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

Merged
merged 5 commits into from
Apr 25, 2019

Conversation

sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented Apr 6, 2019

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)

@choldgraf
Copy link
Collaborator

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
Member Author

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
Copy link
Collaborator

done #200

choldgraf
choldgraf previously approved these changes Apr 7, 2019
Copy link
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
Member Author

@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
Collaborator

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
Collaborator

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
Member Author

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.

Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EEG/iEEG suffixes are not listed in the Entities appendix
6 participants