Skip to content
This repository has been archived by the owner on Apr 2, 2020. It is now read-only.

[MRG] Replace 'paradigm' with 'events', 'experimental paradigm' for BIDS compliance #257

Merged
merged 5 commits into from Oct 8, 2018

Conversation

kchawla-pi
Copy link
Collaborator

issue #216

…radigm' -1

 - Replaced \ line continuation with parantheses.
 - Reformatted multiline ars and expressions.
 - Changed so 'events' is accessed using dict key, not obj attribute.
 - Removed paradigm_from_csv() reference in reference.rst
 (function removed previously)
…mpliance)

 - Replaced .csv file extension for events file with .tsv .
 - In examples, generally replaced paradigm.to_csv('paradigm.csv') with
 events.to_csv('events.tsv', sep='\t', index=False).
 - Tweaked documentation language.
 - Typo correction.
@bthirion
Copy link
Member

bthirion commented Oct 8, 2018

Let me know when it's ready for review.
Best,
B

@kchawla-pi kchawla-pi changed the title [WIP] Replace 'paradigm' with 'events', 'experimental paradigm' for BIDS compliance [MRG] Replace 'paradigm' with 'events', 'experimental paradigm' for BIDS compliance Oct 8, 2018
@kchawla-pi
Copy link
Collaborator Author

It is now ready for review.

@kchawla-pi
Copy link
Collaborator Author

Uh circleci is failing. I'll check that, you can start on the review if you wish.

In some of your examples (newly merged/updated) I have :

  • Replaced .csv file extension for events file with .tsv .
  • In examples, generally replaced paradigm.to_csv('paradigm.csv') with
    events.to_csv('events.tsv', sep='\t', index=False).

Do double check those.

@kchawla-pi
Copy link
Collaborator Author

The error seems similar to what Nilearn experienced: nilearn/nilearn#1763

@kchawla-pi kchawla-pi changed the title [MRG] Replace 'paradigm' with 'events', 'experimental paradigm' for BIDS compliance [Review] Replace 'paradigm' with 'events', 'experimental paradigm' for BIDS compliance Oct 8, 2018
@kchawla-pi
Copy link
Collaborator Author

There's one modification in test_utils.py I forgot to make. I'll push that soon.

 - Renaming localizer_paradigm.csv to localizer_events.tsv caused the error.
 The file is named so on the fetch server and hence cannot be renamed here.
@kchawla-pi
Copy link
Collaborator Author

The error was occurring because I changed the name of the events file from localizer_paradigm.csv to localizer_events.tsv in the code without renaming the file itself first.
I will add that now.

@kchawla-pi
Copy link
Collaborator Author

Hey @bthirion that's file rename is causing problems right now and I think we have better goals to pursue. So I say we merge it this way. Maybe I can open a new PR to rename the events file tomorrow. WHat do you think?

@kchawla-pi kchawla-pi changed the title [Review] Replace 'paradigm' with 'events', 'experimental paradigm' for BIDS compliance [MRG] Replace 'paradigm' with 'events', 'experimental paradigm' for BIDS compliance Oct 8, 2018
Copy link
Member

@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

LGTM overall.

only a simple warning will be displayed. A particular attention should
be given to the 'trial_type' key which defines the different conditions
in the paradigm.
An experimental paradigm is valid if it has an 'onset' key.
Copy link
Member

Choose a reason for hiding this comment

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

and a 'duration' key.

@bthirion
Copy link
Member

bthirion commented Oct 8, 2018

Will merge tonight unless I hear otherwise.
B

@kchawla-pi
Copy link
Collaborator Author

kchawla-pi commented Oct 8, 2018

FYI: In a few places I replaced events key with events columns.

@bthirion
Copy link
Member

bthirion commented Oct 8, 2018

Merging now.

@bthirion bthirion merged commit 35e1cbe into nilearn:master Oct 8, 2018
@kchawla-pi kchawla-pi deleted the events-not-paradigms-3 branch October 13, 2018 07:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants