FIX: Sort conditions in bids_gen_info to ensure consistent order #2867

TheChymera · 2019-01-30T02:03:47Z

@effigies
If this is not done, condition order for orthogonalizarion is
unpredictable.

Acknowledgment

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

codecov-io · 2019-01-30T04:00:16Z

Codecov Report

Merging #2867 into master will decrease coverage by 3.31%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #2867      +/-   ##
==========================================
- Coverage    67.5%   64.19%   -3.32%     
==========================================
  Files         343      341       -2     
  Lines       43598    43546      -52     
  Branches     5426     5424       -2     
==========================================
- Hits        29431    27954    -1477     
- Misses      13465    14517    +1052     
- Partials      702     1075     +373

Flag	Coverage Δ
#smoketests	`?`
#unittests	`64.19% <88.88%> (-0.75%)`	⬇️

Impacted Files	Coverage Δ
nipype/algorithms/modelgen.py	`60.17% <88.88%> (-3.2%)`	⬇️
nipype/interfaces/nilearn.py	`40% <0%> (-56.67%)`	⬇️
nipype/utils/spm_docs.py	`25.92% <0%> (-44.45%)`	⬇️
nipype/interfaces/freesurfer/base.py	`50% <0%> (-30.51%)`	⬇️
nipype/utils/logger.py	`59.7% <0%> (-29.86%)`	⬇️
nipype/algorithms/rapidart.py	`35% <0%> (-29.42%)`	⬇️
nipype/interfaces/spm/base.py	`58.08% <0%> (-29.05%)`	⬇️
nipype/utils/provenance.py	`55.73% <0%> (-28.35%)`	⬇️
nipype/interfaces/fsl/model.py	`55.26% <0%> (-25.35%)`	⬇️
nipype/testing/fixtures.py	`77.33% <0%> (-21.34%)`	⬇️
... and 42 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 08d7092...5fa95f4. Read the comment docs.

effigies

I think this is reasonable. Small suggestion.

@satra I think you suggested there should be a test to go with this?

nipype/algorithms/modelgen.py

satra · 2019-01-30T20:56:32Z

@effigies and @TheChymera - yes, it may be useful to see a point of failure where this will not work. i'm mostly concerned whether this worked and stopped working or did not work properly ever.

also whether this works across both FSL and SPM.

effigies · 2019-01-30T21:11:38Z

This was recently added (#2845) as a BIDS-y alternative to the existing events files, so it's not a failure of a historical construct.

effigies · 2019-02-14T15:49:00Z

@TheChymera Can you review the suggestion? And is there any chance of a small regression test?

effigies · 2019-02-20T14:10:43Z

@TheChymera Just a note that the 1.1.9 release is coming up on Monday, and it would be good to have any PRs we want to include in on Friday.

TheChymera · 2019-02-20T17:10:05Z

@effigies I'm not sure what kind of test you're looking for. Could you provide me with an example?

effigies · 2019-02-20T17:24:08Z

In this case, a simple regression test would be a BIDS-style event file and a function that compares the output of bids_gen_info to an expected output:

def test_bids_gen_info():
    fname = ...
    res = bids_gen_info(fname)
    # Tests of properties
    assert len(res) == 1
    assert res[0].onsets == ...
    assert res[0].durations == ...
    assert res[0].amplitudes == ...
    assert res[0].conditions == ...

TheChymera · 2019-02-21T02:24:19Z

@effigies what directory should I put the example file into? I can't find other example files, e.g. test1.nii anywhere.

satra · 2019-02-21T02:29:46Z

@TheChymera - if you create a test with:

def test_bids_info(tmpdir):
    filename = tmpdir / 'testfile.nii'
    import nibabel as nb
    # create a local file here and use it for a test.
    ....
    img.to_nifti(filename)
    # use filename for test

TheChymera · 2019-02-21T04:26:05Z

@satra so I should create the .tsv file as part of the test? that's a lot more and a lot less legible text than if I tracked it somewhere. But where? examples/? In the test dir?

satra · 2019-02-21T04:50:50Z

the general place where test data go is testing/data but most files there are empty. so you can put small files, but not large ones.

TheChymera · 2019-02-23T01:41:09Z

@satra @effigies works over here, hope this is good now :)

TheChymera · 2019-02-23T20:44:49Z

@satra @effigies it works on my box locally, but I can't get the path to work on circleci. Any advice?

nipype/algorithms/tests/test_modelgen.py

satra · 2019-02-24T16:49:31Z

nipype/algorithms/tests/test_modelgen.py



+def test_bids_gen_info():
+    fname = os.path.realpath('data/events.tsv')


Suggested change

fname = os.path.realpath('data/events.tsv')

fname = npt.example_data('events.tsv')

sorry i forgot that location is not a default in the regular tests.

effigies

Two minor style points, but otherwise this looks good. Looks like @satra's fix should fix up the tests, but let me know if we're still falling short.

nipype/algorithms/modelgen.py

TheChymera · 2019-02-25T04:54:28Z

Added separate import because @satra's suggestion failed with

def test_bids_gen_info():
>       fname = npt.example_data('events.tsv')
E       AttributeError: module 'numpy.testing' has no attribute 'example_data'

/src/nipype/nipype/algorithms/tests/test_modelgen.py:20: AttributeError

TheChymera · 2019-02-25T06:19:53Z

Ok, can't reproduce this new failure locally, sounds like some unicode support is missing from the testing environment:

def test_bids_gen_info():
        fname = example_data('events.tsv')
>       res = bids_gen_info([fname])

/src/nipype/nipype/algorithms/tests/test_modelgen.py:22: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/src/nipype/nipype/algorithms/modelgen.py:177: in bids_gen_info
    f_events = csv.DictReader(f, skipinitialspace=True, delimiter='\t')
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <csv.DictReader instance at 0x7f4a4d694d88>
f = <closed file '/src/nipype/nipype/testing/data/events.tsv', mode 'r' at 0x7f4a4d943300>
fieldnames = None, restkey = None, restval = None, dialect = 'excel', args = ()
kwds = {'delimiter': '	', 'skipinitialspace': True}

    def __init__(self, f, fieldnames=None, restkey=None, restval=None,
                 dialect="excel", *args, **kwds):
        self._fieldnames = fieldnames   # list of keys for the dict
        self.restkey = restkey          # key to catch long rows
        self.restval = restval          # default value for short rows
>       self.reader = reader(f, dialect, *args, **kwds)
E       TypeError: "delimiter" must be string, not unicode

/opt/miniconda-latest/envs/neuro/lib/python2.7/csv.py:79: TypeError

effigies

Looks like you found the nipype.testing fix. Added a suggestion for your Py2 issue.

And can you rebase on top of current master to get all imports and resolve the merge conflict?

nipype/algorithms/modelgen.py

If this is not done, condition order for orthogonalizarion is unpredictable.

TheChymera · 2019-02-25T21:33:56Z

@effigies

ok, so using the str type from builtins gives me this:

E       TypeError: "delimiter" must be string, not newstr

The only way I was able to fix it is by re-assigning the base type before the import, since removing the import breaks another function.

effigies

Hmm. That's annoying, but I don't see another way around it. @satra @oesteban Do either of you recall if we had to deal with needing Python 2's built-in str + builtins.str somewhere else? It'd be good to be consistent with our convention.

Either way, this LGTM.

effigies · 2019-03-01T01:16:42Z

Going to go ahead and merge. Thanks for your patience!

effigies reviewed Jan 30, 2019

View reviewed changes

nipype/algorithms/modelgen.py Outdated Show resolved Hide resolved

effigies changed the title ~~BF: unambiguous conditions order~~ FIX: Sort conditions in bids_gen_info to ensure consistent order Feb 14, 2019

TheChymera force-pushed the order branch from e05db14 to ea96591 Compare February 23, 2019 01:40

TheChymera force-pushed the order branch 2 times, most recently from b433508 to 58737df Compare February 23, 2019 06:45

satra reviewed Feb 23, 2019

View reviewed changes

nipype/algorithms/tests/test_modelgen.py Outdated Show resolved Hide resolved

TheChymera force-pushed the order branch from 58737df to eaeeb45 Compare February 24, 2019 04:23

satra reviewed Feb 24, 2019

View reviewed changes

effigies reviewed Feb 24, 2019

View reviewed changes

nipype/algorithms/modelgen.py Outdated Show resolved Hide resolved

nipype/algorithms/modelgen.py Outdated Show resolved Hide resolved

effigies added this to the 1.1.9 milestone Feb 24, 2019

TheChymera force-pushed the order branch 3 times, most recently from f8e0964 to d9b8378 Compare February 25, 2019 04:53

effigies reviewed Feb 25, 2019

View reviewed changes

nipype/algorithms/modelgen.py Outdated Show resolved Hide resolved

effigies removed this from the 1.1.9 milestone Feb 25, 2019

effigies added this to the 1.2.0 milestone Feb 25, 2019

TheChymera force-pushed the order branch 2 times, most recently from 17bea46 to f627f4d Compare February 25, 2019 19:24

BF: tests and unambiguous conditions order

5fa95f4

If this is not done, condition order for orthogonalizarion is unpredictable.

TheChymera force-pushed the order branch from f627f4d to 5fa95f4 Compare February 25, 2019 21:33

effigies approved these changes Feb 26, 2019

View reviewed changes

effigies merged commit 232ac23 into nipy:master Mar 1, 2019

TheChymera deleted the order branch March 1, 2019 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Sort conditions in bids_gen_info to ensure consistent order #2867

FIX: Sort conditions in bids_gen_info to ensure consistent order #2867

TheChymera commented Jan 30, 2019 •

edited

Loading

codecov-io commented Jan 30, 2019 •

edited

Loading

effigies left a comment

satra commented Jan 30, 2019

effigies commented Jan 30, 2019

effigies commented Feb 14, 2019

effigies commented Feb 20, 2019

TheChymera commented Feb 20, 2019

effigies commented Feb 20, 2019

TheChymera commented Feb 21, 2019

satra commented Feb 21, 2019

TheChymera commented Feb 21, 2019

satra commented Feb 21, 2019

TheChymera commented Feb 23, 2019

TheChymera commented Feb 23, 2019

satra Feb 24, 2019

satra Feb 24, 2019

effigies left a comment

TheChymera commented Feb 25, 2019

TheChymera commented Feb 25, 2019

effigies left a comment

TheChymera commented Feb 25, 2019

effigies left a comment

effigies commented Mar 1, 2019



		def test_bids_gen_info():
		fname = os.path.realpath('data/events.tsv')

	fname = os.path.realpath('data/events.tsv')
	fname = npt.example_data('events.tsv')

FIX: Sort conditions in bids_gen_info to ensure consistent order #2867

FIX: Sort conditions in bids_gen_info to ensure consistent order #2867

Conversation

TheChymera commented Jan 30, 2019 • edited Loading

Acknowledgment

codecov-io commented Jan 30, 2019 • edited Loading

Codecov Report

effigies left a comment

Choose a reason for hiding this comment

satra commented Jan 30, 2019

effigies commented Jan 30, 2019

effigies commented Feb 14, 2019

effigies commented Feb 20, 2019

TheChymera commented Feb 20, 2019

effigies commented Feb 20, 2019

TheChymera commented Feb 21, 2019

satra commented Feb 21, 2019

TheChymera commented Feb 21, 2019

satra commented Feb 21, 2019

TheChymera commented Feb 23, 2019

TheChymera commented Feb 23, 2019

satra Feb 24, 2019

Choose a reason for hiding this comment

satra Feb 24, 2019

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

TheChymera commented Feb 25, 2019

TheChymera commented Feb 25, 2019

effigies left a comment

Choose a reason for hiding this comment

TheChymera commented Feb 25, 2019

effigies left a comment

Choose a reason for hiding this comment

effigies commented Mar 1, 2019

TheChymera commented Jan 30, 2019 •

edited

Loading

codecov-io commented Jan 30, 2019 •

edited

Loading