Skip to content

small changes in FSSourceInputSpec (fixes #2116) #2349

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 6 commits into from
Jan 18, 2018

Conversation

djarecka
Copy link
Collaborator

adding exists=True for subjects_dir in FSSourceInputSpec
fixes #2116

Copy link
Contributor

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Please also make specs and commit the updated auto tests.

@@ -407,6 +407,13 @@ def test_freesurfersource():
assert fss.inputs.subjects_dir == Undefined


def test_freesurfersource_incorrectdir():
fss = nio.FreeSurferSource()
with pytest.raises(Exception) as err:
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't it work using with pytest.raises(TraitError) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you're right, after importing TraitError it works...

@oesteban
Copy link
Contributor

oesteban commented Jan 3, 2018

this is a bit weird: why the only changing specs are unrelated to freesurfer? I'd expect FS' interfaces to change... WDYT?

@djarecka
Copy link
Collaborator Author

djarecka commented Jan 3, 2018

@oesteban - I assumed that it was not run in previous PR for some time, but could be wrong... in my other PR (#2350) I also had changes in those files after running make specs, and this was partially the reason why I didn't run it here

@oesteban
Copy link
Contributor

oesteban commented Jan 3, 2018

Maybe @satra will correct me, but the idea for the auto tests is to be updated with the PR that modifies interfaces, so that we clearly see how the interface changed. Is that correct?

If so, then we would need to keep both PRs separate and update specs accordingly.

@djarecka
Copy link
Collaborator Author

djarecka commented Jan 3, 2018

@oesteban - not sure if I understand your point. I didn't mix my PRs, I only wrote that I had exactly the same problem in my other PR, i.e., make specs changed autotests that were not related to my changes. In both cases two autotests were deleted: nipype/interfaces/ants/tests/test_auto_Registration.py and nipype/interfaces/niftyseg/tests/test_auto_PatchMatch.py.

My guess was that simply make specs were not run with the PRs that changed those 2 interfaces, so I just included the changes. I've just checked that make specs removes exactly the same files even when run in my master (with clean working tree). Can you please check it on your master, I might be doing something wrong...

@djarecka
Copy link
Collaborator Author

@oesteban - the mystery solved, make specs works differently on my OSX (is not case sensitive). @effigies is addressing this issue in #2374

This reverts commit ba9ae4f.
@effigies
Copy link
Member

Is this ready to merge? @oesteban @djarecka

@djarecka
Copy link
Collaborator Author

@effigies yes

@effigies effigies merged commit 80d3f05 into nipy:master Jan 18, 2018
@effigies effigies modified the milestones: 0.14.1, 1.0 Jan 19, 2018
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.

FreeSurferSource doesn't check subjects_dir
3 participants