cpp-lln-lab / CPP_BIDS

a set of matlab / octave function to create BIDS comptible folder structure and filenames
https://cpp-bids.readthedocs.io/en/dev/index.html
MIT License
1 stars 9 forks source link

enable setting column Levels properties #115

Closed TomasLenc closed 3 years ago

TomasLenc commented 4 years ago

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 commented 4 years ago

@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):

TomasLenc commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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. :wink: No pressure.

TomasLenc commented 4 years ago

I promise will fix at the tests tomorrow.

Remi-Gau commented 3 years ago

@all-contributors please add @TomasLenc code, ideas

allcontributors[bot] commented 3 years ago

@Remi-Gau

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

TomasLenc commented 3 years ago

@Remi-Gau

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

Feeling honoured

Remi-Gau commented 3 years ago

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. :wink:

Remi-Gau commented 3 years ago

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.

codecov[bot] commented 3 years ago

Codecov Report

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

Impacted file tree graph

@@            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%) :arrow_down:

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%) :arrow_down:
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 commented 3 years ago

@all-contributors please add @TomasLenc test

allcontributors[bot] commented 3 years ago

@Remi-Gau

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