Skip to content
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

[Fix] Diffeormorphic + CCMetric on small image #1819

Merged
merged 5 commits into from May 5, 2019

Conversation

@skoudoro
Copy link
Member

commented May 3, 2019

This PR is just an implementation of this comment. It should fix #1048.

@skoudoro skoudoro added this to the 1.0 milestone May 3, 2019

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

@grlee77, Can you have a look at this PR when you find the time?

Thank you! 😄

@grlee77
Copy link
Contributor

left a comment

Hi @skoudoro
I had forgotten about this. Thank you for making a PR.

Overall it looks good, but I have suggested some minor revisions.

return True if len(valid_shape) != image.ndim else False

msg = ("Each image dimensions should be superior to 2 * radius + 1."
"Decrease CCMetric radius or Increase your image size")

This comment has been minimized.

Copy link
@grlee77

grlee77 May 4, 2019

Contributor

dimensions -> dimension
Increase -> increase

dipy/align/metrics.py Show resolved Hide resolved
if invalid_image_size(self.static_image):
raise IOError("Static image size is too small. " + msg)
if invalid_image_size(self.moving_image):
raise IOError("Moving image size is too small. " + msg)

This comment has been minimized.

Copy link
@grlee77

grlee77 May 4, 2019

Contributor

I would have expected ValueError or RuntimeError rather than IOError, but I'm not familiar with what convention DiPy tends to favor.

test_exceptions()
test_EMMetric_image_dynamics()
# test_EMMetric_image_dynamics()

This comment has been minimized.

Copy link
@grlee77

grlee77 May 4, 2019

Contributor

Why have these other tests been disabled? Is this left over from when you were debugging?

This comment has been minimized.

Copy link
@skoudoro

skoudoro May 4, 2019

Author Member

left over, I will correct that...

assert_raises(IOError, metric.initialize_iteration)

metric = CCMetric(2)
for shape in itertools.product((5, 8), (8, 5)):

This comment has been minimized.

Copy link
@grlee77

grlee77 May 4, 2019

Contributor

it will be clearer to others in the future if you add a small comment:

# expected to fail for any dimension < 2*radius + 1.

and then explicitly set the radius argument to CCMetric.

For completeness, you could also check that it does not raise when size == 2*radius + 1.

This comment has been minimized.

Copy link
@skoudoro

skoudoro May 4, 2019

Author Member

Agree, done in my last commit

@codecov-io

This comment has been minimized.

Copy link

commented May 4, 2019

Codecov Report

Merging #1819 into master will decrease coverage by <.01%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1819      +/-   ##
==========================================
- Coverage   83.92%   83.92%   -0.01%     
==========================================
  Files         120      120              
  Lines       14570    14579       +9     
  Branches     2295     2298       +3     
==========================================
+ Hits        12228    12235       +7     
- Misses       1823     1824       +1     
- Partials      519      520       +1
Impacted Files Coverage Δ
dipy/align/metrics.py 96.1% <77.77%> (-0.48%) ⬇️
@skoudoro

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

Thank you for your Nice review @grlee77. I just addressed all your comment

@grlee77

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

Thanks for addressing the comments. This looks good to me (assuming the CI passes)

@arokem

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Looks good to me. CI failure is unrelated, so I will go ahead and merge.

@arokem arokem merged commit 47b3541 into nipy:master May 5, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@skoudoro skoudoro deleted the skoudoro:fix-1048 branch May 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.