enable setting column Levels properties #115

TomasLenc · 2020-10-23T12:18:37Z

Changed order in which things are done when initializing. Now, all
the "holy trinity" columns and user-specified extra columns
are saved in the structure after calling saveEventsFile('init').

Next, the user can edit column properties before calling
saveEventsFile('open') which will write the JSON.

This way, Levels of e.g. trial_type can be set by the user. Moreover,
this gives the user the freedom to set the Levels as containers.Map
data type, which describes mapping between each level name and its
corresponding description. This is super useful because sometimes
levels can have cryptic names like '1' and '2', and so it's handy
to explain what each code means (e.g. 'left' and 'right' respectively).

Here is an example:

% allocate
logFile = [];

% We can define what extra columns we want in our tsv file beyond the
% BIDS holy trinity ('onset', 'duration', 'trial_type')
logFile.extraColumns = {'rhythm', 'trigger'};

% init logfile
logFile = saveEventsFile('init',cfg,logFile);

% change info about trial_type
logFile.columns.trial_type.Levels = containers.Map({'listen','tap'}, ...
                                                   {'listen without movement','tap finger with the pulse'});

% change units to miliseconds
logFile.columns.onset.Unit = 'ms';
logFile.columns.duration.Unit = 'ms';

% add info to the extra columns
logFile.extraColumns.rhythm.bids.Description = 'name of the rhythmic pattern';
logFile.extraColumns.rhythm.bids.Levels = {'unsyncopated','syncopated'};

logFile.extraColumns.trigger.bids.Description = 'EEG trigger value';
logFile.extraColumns.trigger.bids.Levels = containers.Map({'1','2'},{'low tone','high tone'});

% open tsv and write json for events
logFile = saveEventsFile('open',cfg,logFile);

I hope this makes sense. I'm not sure about the _stim file bids rules, so I tried not to mess things up too much.

Remi-Gau · 2020-10-23T13:03:35Z

@TomasLenc

Thanks for this. Will try to have a look at it this weekend to give you some feedback.

Adding some tick boxes below, but that's more for me to remember to do those on the PR later (though you are more than welcome to give it a go if you fancy):

update unit tests till the continuous integration of Travis passes
apply linter to make miss_hit happy
update jupyter notebook with example

TomasLenc · 2020-10-24T11:21:23Z

@Remi-Gau thanks, let me know what you think of the proposed changes. If you decide they make sense, I'm happy to update the notebooks with examples.

And sorry for the failed checks...I need to learn more about how this works!

Remi-Gau · 2020-10-27T21:34:03Z

Finally taking the time to look into this.

I like the way this is going. Just a couple of point before I start an "actual" review.

tsv files have no header rows

I think there is one thing that this does is that this adds a header row to the TSV files of the stim files which is not OK by the BIDS standard.

This is because openFile calls printHeaderExtraColumns no matter if the file is a an event or a stim file.

A way to deal with this would be to create a isStim boolean field in the logFile structure and to set it to True or False depending on the initialization used.

Then printHeaderExtraColumns would only run is isStim is false.

stim files have no "holy trinity"

The other problem is that this stim files are NOT required to have onsets, durations and trial_type columns and this would make those fields "obligatory".

https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/06-physiological-and-other-continuous-recordings.html

So in this case too we could rely on logFile.isStim to decide what initializeFile should put in the logFile.columns.

Does that make sense?

Remi-Gau · 2020-10-28T07:15:32Z

update help section

by the way @TomasLenc if you want me to guide you on how to work with the unit tests and how you can use them to see what other changes will need to be added to the code, let me know. More than happy to explain it.

TomasLenc · 2020-10-29T15:27:08Z

Thanks for the feedback @Remi-Gau, I've looked up the rules for _stim file and changed the way the .tsv is saved. As you suggested, there is now a "isStim" field that controls the behavior depending on whether the file was initialized as _events or _stim.

Not sure what your plan was regarding the actual saving within the _stim.tsv file. Assuming you may want to use the same idea as for the _events.tsv file, I patched parts of the code to make it possible (even when saving "event-by-event"). The way I did it is not very beautiful, so feel free to reject this :D

TomasLenc · 2020-10-29T15:32:51Z

by the way @TomasLenc if you want me to guide you on how to work with the unit tests and how you can use them to see what other changes will need to be added to the code, let me know. More than happy to explain it.

Thanks, that would be super nice cause it hurts my heart to see everything failing after my PR :(( Did you want to link me to some tutorial, or perhaps you may have time to chat? Let me know, I'm aware you're busy!

Remi-Gau · 2020-10-29T15:39:55Z

by the way @TomasLenc if you want me to guide you on how to work with the unit tests and how you can use them to see what other changes will need to be added to the code, let me know. More than happy to explain it.

Thanks, that would be super nice cause it hurts my heart to see everything failing after my PR :(( Did you want to link me to some tutorial, or perhaps you may have time to chat? Let me know, I'm aware you're busy!

We can chat tomorrow if you have time.

In the mean time you can have a look at this.
https://the-turing-way.netlify.app/reproducible-research/testing.html

Actually they have expanded it so I should probably have a look too.

Remi-Gau · 2020-10-29T15:40:21Z

Thanks for the feedback @Remi-Gau, I've looked up the rules for _stim file and changed the way the .tsv is saved. As you suggested, there is now a "isStim" field that controls the behavior depending on whether the file was initialized as _events or _stim.

Not sure what your plan was regarding the actual saving within the _stim.tsv file. Assuming you may want to use the same idea as for the _events.tsv file, I patched parts of the code to make it possible (even when saving "event-by-event"). The way I did it is not very beautiful, so feel free to reject this :D

Will try to check it tonight.

Remi-Gau

Here is my review.

A couple of things need to be changed.

Some others are a bit more up to you.

Then there are some possibility to refactor the code in a couple of places.

src/saveEventsFile.m

src/createDataDictionary.m

src/saveEventsFile.m

Remi-Gau · 2020-11-02T09:08:27Z

@TomasLenc I have opened another PR #119

The plan for this one is just some doc to help people contribute to this repo.

I think we should have a section on writing / using code tests.

So if you have time to help with this. 😉 No pressure.

TomasLenc · 2020-11-02T23:51:05Z

I promise will fix at the tests tomorrow.

Remi-Gau

Couple of things to change. Nothing big.

src/createDataDictionary.m

src/saveEventsFile.m

src/createDataDictionary.m

src/saveEventsFile.m

Remi-Gau · 2020-11-09T09:25:38Z

@all-contributors please add @TomasLenc code, ideas

allcontributors · 2020-11-09T09:25:46Z

@Remi-Gau

I've put up a pull request to add @TomasLenc! 🎉

TomasLenc · 2020-11-16T00:01:03Z

@Remi-Gau

I've put up a pull request to add @TomasLenc! 🎉

Feeling honoured

Changed order in which things are done when initializing. Now, all the "holy trinity" columns and user-specified extra columns are saved in the structure after calling saveEventsFile('init'). Next, the user can edit column properties before calling saveEventsFile('open') which will write the JSON. This way, Levels of e.g. trial_type can be set by the user. Moreover, this gives the user the freedom to set the Levels as containers.Map data type, which describes mapping between each level name and its corresponding description. This is super useful because sometimes levels can have cryptic names like '1' and '2', and so it's handy to explain what each code means (e.g. 'left' and 'right' respectively). Here is an example: % allocate logFile = []; % We can define what extra columns we want in our tsv file beyond the % BIDS holy trinity ('onset', 'duration', 'trial_type') logFile.extraColumns = {'rhythm', 'trigger'}; % init logfile logFile = saveEventsFile('init',cfg,logFile); % change info about trial_type logFile.columns.trial_type.Levels = containers.Map({'listen','tap'}, ... {'listen without movement','tap finger with the pulse'}); % change units to miliseconds logFile.columns.onset.Unit = 'ms'; logFile.columns.duration.Unit = 'ms'; % add info to the extra columns logFile.extraColumns.rhythm.bids.Description = 'name of the rhythmic pattern'; logFile.extraColumns.rhythm.bids.Levels = {'unsyncopated','syncopated'}; logFile.extraColumns.trigger.bids.Description = 'EEG trigger value'; logFile.extraColumns.trigger.bids.Levels = containers.Map({'1','2'},{'low tone','high tone'}); % open tsv and write json for events logFile = saveEventsFile('open',cfg,logFile);

Additional field .isStim is created in the logFile structure upon initialization (true if _stim file is requested, false otherwise). If _stim file is requested, no headers are printed in the .tsv file, and the standard columns "onset", "duration", "trial_type" are omitted.

Changes based on Remi-Gau's comments.

All unit tests are working with the updated functions. Also, applied some small changes requested in the review process for the PR.

Remi-Gau · 2020-11-16T21:35:48Z

hey @TomasLenc I did a rebase of this PR on the dev branch to fix some merge conflict. So make sure you pull first before making any changes. 😉

Remi-Gau · 2020-11-16T22:24:00Z

This saveEventsFile will require additional tests to define how it behaves when saving stim files but I am tempted to move that into another PR since the test suite we have requires quite a bit of refactoring anyway.

Remi-Gau

OK I am happy with this. I think that the related issues (continuous integration, documentation, jupyter book...) should be addressed in other PRs.

codecov · 2020-11-16T23:10:29Z

Codecov Report

Merging #115 (91b2260) into dev (af811a0) will decrease coverage by 2.39%.
The diff coverage is 76.56%.

@@            Coverage Diff             @@
##              dev     #115      +/-   ##
==========================================
- Coverage   81.06%   78.67%   -2.40%     
==========================================
  Files          29       29              
  Lines         618      647      +29     
==========================================
+ Hits          501      509       +8     
- Misses        117      138      +21

Flag	Coverage Δ
unittests	`78.67% <76.56%> (-2.40%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/convertSourceToRaw.m	`0.00% <ø> (ø)`
src/createDatasetDescription.m	`0.00% <ø> (-100.00%)`	⬇️
src/createFilename.m	`95.89% <ø> (ø)`
src/createJson.m	`51.21% <ø> (ø)`
src/gui/askForGroupAndOrSession.m	`100.00% <ø> (ø)`
src/gui/askUserCli.m	`0.00% <ø> (ø)`
src/gui/askUserGui.m	`0.00% <ø> (ø)`
src/gui/createQuestionList.m	`100.00% <ø> (ø)`
src/gui/getIsQuestionToAsk.m	`100.00% <ø> (ø)`
src/gui/setDefaultResponses.m	`100.00% <ø> (ø)`
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 435eb3d...91b2260. Read the comment docs.

Remi-Gau · 2020-11-17T08:03:18Z

@all-contributors please add @TomasLenc test

allcontributors · 2020-11-17T08:03:27Z

@Remi-Gau

I've put up a pull request to add @TomasLenc! 🎉

Remi-Gau requested changes Oct 30, 2020

View reviewed changes

Remi-Gau requested changes Nov 8, 2020

View reviewed changes

Remi-Gau reviewed Nov 8, 2020

View reviewed changes

src/saveEventsFile.m Outdated Show resolved Hide resolved

Remi-Gau reviewed Nov 8, 2020

View reviewed changes

src/saveEventsFile.m Outdated Show resolved Hide resolved

allcontributors bot mentioned this pull request Nov 9, 2020

docs: add TomasLenc as a contributor #122

Merged

Remi-Gau mentioned this pull request Nov 16, 2020

update doc on saveEvents #137

Merged

TomasLenc added 4 commits November 16, 2020 22:33

Small changes

3859e60

Changes based on Remi-Gau's comments.

updated tests and small changes

98d8ccf

All unit tests are working with the updated functions. Also, applied some small changes requested in the review process for the PR.

Remi-Gau force-pushed the tomas-dev branch from 4f1a9cb to 98d8ccf Compare November 16, 2020 21:34

mh autofix

72b5768

Remi-Gau mentioned this pull request Nov 16, 2020

update saveEvents doc and jupyter notebook #140

Closed

2 tasks

Remi-Gau mentioned this pull request Nov 16, 2020

Add test for saveEventsFile for saving stim files #142

Closed

update test makeRawDataset to adapt the new behavior of saveEventFile

dac6de3

Remi-Gau approved these changes Nov 16, 2020

View reviewed changes

move test createDataDictionary to manual test because of CI issue

7bfac47

Remi-Gau force-pushed the tomas-dev branch from b0848f3 to 7bfac47 Compare November 16, 2020 22:45

Remi-Gau added 2 commits November 17, 2020 00:07

also move createDatasetDesciption into the manual tests

25831f2

update bids-matlab submodule

91b2260

Remi-Gau merged commit 0cda77c into cpp-lln-lab:dev Nov 16, 2020

allcontributors bot mentioned this pull request Nov 17, 2020

docs: add TomasLenc as a contributor #143

Merged

enable setting column Levels properties #115

enable setting column Levels properties #115

Uh oh!

Conversation

TomasLenc commented Oct 23, 2020

Uh oh!

Remi-Gau commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomasLenc commented Oct 24, 2020

Uh oh!

Remi-Gau commented Oct 27, 2020

tsv files have no header rows

stim files have no "holy trinity"

Uh oh!

Remi-Gau commented Oct 28, 2020

Uh oh!

TomasLenc commented Oct 29, 2020

Uh oh!

TomasLenc commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Remi-Gau commented Oct 29, 2020

Uh oh!

Remi-Gau commented Oct 29, 2020

Uh oh!

Remi-Gau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Remi-Gau commented Nov 2, 2020

Uh oh!

TomasLenc commented Nov 2, 2020

Uh oh!

Remi-Gau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Remi-Gau commented Nov 9, 2020

Uh oh!

allcontributors bot commented Nov 9, 2020

Uh oh!

TomasLenc commented Nov 16, 2020

Uh oh!

Remi-Gau commented Nov 16, 2020

Uh oh!

Remi-Gau commented Nov 16, 2020

Uh oh!

Remi-Gau left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Remi-Gau commented Nov 17, 2020

Uh oh!

allcontributors bot commented Nov 17, 2020

Uh oh!

Reviewers

Remi-Gau commented Oct 23, 2020 •

edited

Loading

TomasLenc commented Oct 29, 2020 •

edited

Loading

codecov bot commented Nov 16, 2020 •

edited

Loading