Skip to content

[FIX] Fixed a small bug in freesurfer label2annot fill_thresh specs #2377

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 11 commits into from
Feb 24, 2018

Conversation

achetverikov
Copy link
Contributor

Fixes a small bug in freesurfer label2annot fill_thresh specs (should be %f instead of %.f)

@effigies
Copy link
Member

Please merge master, run make specs, and commit the updated auto tests.

@satra
Copy link
Member

satra commented Feb 15, 2018

@achetverikov - just checking to see if you can:

merge master, run make specs, and commit the updated auto tests

@achetverikov
Copy link
Contributor Author

will do, at least will try to

@effigies
Copy link
Member

Hi @achetverikov do you have a second to give this a shot? Just to be explicit about what I was suggesting:

  1. Make sure you have a remote pointing to the main repository (often called upstream):
git remote add upstream https://github.com/nipy/nipype.git
  1. Fetch upstream and merge upstream/master
git fetch upstream
git merge upstream/master
  1. Generate auto tests, add and push
make specs
git add nipype/interfaces/freesurfer/tests/*
git commit -m 'TEST: make specs'
git push

@achetverikov
Copy link
Contributor Author

OK, done. Thanks for the detailed instructions, @effigies .

@@ -1,6 +1,6 @@
# AUTO-GENERATED by tools/checkspecs.py - DO NOT EDIT
from __future__ import unicode_literals
from ..model import Binarize
from ..model_bkp import Binarize
Copy link
Member

Choose a reason for hiding this comment

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

Something seems to have gone wrong. I'm guessing model_bkp.py is a backup file you have in your local repository? That's interfering with the proper behavior of make specs. Could you remove it, re-run make specs and add the updated auto tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right. I forgot that I made a backup and was looking at the error message with no clue to what's going on. I did as you suggested, hope that will work.

@codecov-io
Copy link

codecov-io commented Feb 23, 2018

Codecov Report

Merging #2377 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2377   +/-   ##
=======================================
  Coverage   66.66%   66.66%           
=======================================
  Files         328      328           
  Lines       42551    42551           
  Branches     5276     5276           
=======================================
  Hits        28366    28366           
  Misses      13505    13505           
  Partials      680      680
Flag Coverage Δ
#smoketests 50.76% <ø> (ø) ⬆️
#unittests 63.98% <ø> (ø) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/freesurfer/model.py 64.25% <ø> (ø) ⬆️

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 52efaa0...bbe7f69. Read the comment docs.

effigies
effigies previously approved these changes Feb 23, 2018
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.

Looks good. Will merge when tests pass.

@effigies effigies added this to the 1.0.1 milestone Feb 23, 2018
@@ -1144,7 +1144,7 @@ class Label2VolInputSpec(FSTraitedSpec):
invert_mtx = traits.Bool(
argstr='--invertmtx', desc='Invert the registration matrix')
fill_thresh = traits.Range(
0., 1., argstr='--fillthresh %.f', desc='thresh : between 0 and 1')
0., 1., argstr='--fillthresh %f', desc='thresh : between 0 and 1')
Copy link
Member

Choose a reason for hiding this comment

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

Let's go ahead and change this to %g (and remake auto tests).

Also, in the Label2Vol docstring, you'll need to change --fillthresh 0 to --fillthresh 0.5.

@effigies effigies dismissed their stale review February 23, 2018 16:43

Docstring issue

@effigies
Copy link
Member

Hi @achetverikov, do you have a couple minutes to finish this off? I think it's just about done.

@achetverikov
Copy link
Contributor Author

right, I added the changes you suggested merged with master again, ran make specs, and so on
I hope I didn't do anything that shouldn't have been done

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.

Restarted a test. Should be good to go.

@effigies effigies merged commit e1bd536 into nipy:master Feb 24, 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.

4 participants