Skip to content

ENH: Support for BIDS event files #2845

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

Merged
merged 1 commit into from
Jan 21, 2019
Merged

ENH: Support for BIDS event files #2845

merged 1 commit into from
Jan 21, 2019

Conversation

TheChymera
Copy link
Collaborator

SpecifyModel now supports BIDS event files as inputs.

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

@effigies
Copy link
Member

Just a heads up that we're working on putting all of this functionality into pybids and FitLins (see LoadBIDSModel, which will produce outputs similar to SpecifyModel and we'll need to make adapted FSL, SPM and AFNI interfaces to handle the differences). The interfaces will be upstreamed to nipype when stabilized.

That said, this is a pretty light-weight addition that will make adapting existing pipelines to handle BIDS-formatted inputs easy, especially in the absence of BIDS stats models, which is still a draft spec.

Can you merge master to fix the failing test?

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Mostly style issues, but it would also be good to add a test with the expected outputs for a simple file. Perhaps try durations and repetition times that have the potential to hit Python 2/3 differences.

$ python2.7 -c 'print(round(2.5, 0))'
3.0
$ python3.6 -c 'print(round(2.5, 0))'
2.0

np.round is consistent across versions, and consistent with Python 3, evenly rounding.

@@ -144,6 +144,61 @@ def scale_timings(timelist, input_units, output_units, time_repetition):
timelist = [np.max([0., _scalefactor * t]) for t in timelist]
return timelist

def bids_gen_info(bids_event_files,
condition_column='trial_type',
Copy link
Member

Choose a reason for hiding this comment

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

The space before each parameter should align with the open paren:

Suggested change
condition_column='trial_type',
condition_column='trial_type',

@@ -144,6 +144,61 @@ def scale_timings(timelist, input_units, output_units, time_repetition):
timelist = [np.max([0., _scalefactor * t]) for t in timelist]
return timelist

def bids_gen_info(bids_event_files,
condition_column='trial_type',
amplitude_column=None,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
amplitude_column=None,
amplitude_column=None,

condition_column='trial_type',
amplitude_column=None,
time_repetition=False,
):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
):

with open(bids_event_file) as f:
f_events = csv.DictReader(f, skipinitialspace=True, delimiter='\t')
events = [{k: v for k, v in row.items()} for row in f_events]
conditions = list(set([i[condition_column] for i in events]))
Copy link
Member

Choose a reason for hiding this comment

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

You can release the file handle as soon as it has been fully read.

Suggested change
conditions = list(set([i[condition_column] for i in events]))
conditions = list(set([i[condition_column] for i in events]))

amplitudes = [float(i[amplitude_column]) for i in selected_events]
runinfo.amplitudes.append(amplitudes)
except KeyError:
runinfo.amplitudes.append([1]*len(onsets))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
runinfo.amplitudes.append([1]*len(onsets))
runinfo.amplitudes.append([1] * len(onsets))

@codecov-io
Copy link

codecov-io commented Jan 14, 2019

Codecov Report

Merging #2845 into master will decrease coverage by 0.03%.
The diff coverage is 11.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2845      +/-   ##
==========================================
- Coverage   67.44%   67.41%   -0.04%     
==========================================
  Files         341      341              
  Lines       43393    43427      +34     
  Branches     5382     5396      +14     
==========================================
+ Hits        29268    29277       +9     
- Misses      13427    13453      +26     
+ Partials      698      697       -1
Flag Coverage Δ
#smoketests 50.46% <11.76%> (-0.06%) ⬇️
#unittests 64.83% <11.76%> (-0.02%) ⬇️
Impacted Files Coverage Δ
nipype/algorithms/modelgen.py 63.36% <11.76%> (-4.08%) ⬇️
nipype/pipeline/plugins/legacymultiproc.py 66% <0%> (+2.5%) ⬆️

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 4468b3a...e88a825. Read the comment docs.

@effigies
Copy link
Member

@TheChymera Can you merge/rebase current master?

@TheChymera
Copy link
Collaborator Author

@effigies ok :)

@effigies effigies merged commit dedf5ca into master Jan 21, 2019
@effigies effigies deleted the model branch January 21, 2019 02:43
yarikoptic added a commit to yarikoptic/nipype that referenced this pull request Feb 4, 2019
1.1.8 (January 28, 2019)

  * FIX: ANTS LaplacianThickness cmdline opts fixed up (nipy#2846)
  * FIX: Resolve LinAlgError during SVD (nipy#2838)
  * ENH: Add interfaces wrapping DIPY worflows (nipy#2830)
  * ENH: Update BIDSDataGrabber for pybids 0.7 (nipy#2737)
  * ENH: Add FSL `eddy_quad` interface (nipy#2825)
  * ENH: Support tckgen -select in MRtrix3 v3+ (nipy#2823)
  * ENH: Support for BIDS event files (nipy#2845)
  * ENH: CompositeTransformUtil, new ANTs interface (nipy#2785)
  * RF: Move pytest and pytest-xdist from general requirement into tests_required (nipy#2850)
  * DOC: Add S3DataGrabber example (nipy#2849)
  * DOC: Skip conftest module in API generation (nipy#2852)
  * DOC: Hyperlink DOIs to preferred resolver (nipy#2833)
  * MAINT: Install numpy!=1.16.0 from conda in Docker (nipy#2862)
  * MAINT: Drop pytest-xdist requirement, minimum pytest version  (nipy#2856)
  * MAINT: Disable numpy 1.16.0 for Py2.7 (nipy#2855)

* tag '1.1.8': (79 commits)
  MNT: Add @feilong to .zenodo, update ordering
  MNT: Update .mailmap
  MNT: Update .zenodo ordering
  Accept invitation as Zenodo release co-author (see nipy#2864)
  MAINT: Update .mailmap
  BF: allowing bids_event_file as alternate input
  MNT: Update .zenodo ordering
  MNT: Version 1.1.8
  DOC: 1.1.8 changelog
  Update nipype/interfaces/dipy/tracks.py
  Update nipype/interfaces/dipy/reconstruction.py
  MNT: Install numpy!=1.16.0 from conda in Docker
  Add FSL auto test
  remake specs
  Update nipype/interfaces/io.py
  Remove return type named tuple
  Update nipype/info.py
  STY: Whitespace, line length
  Remove out_ prefix from EddyQuad outputs
  Apply minor edits from code review
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants