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 diffeomorphic registration test failures #489

Merged
merged 12 commits into from Dec 8, 2014

Conversation

Projects
None yet
3 participants
@omarocegueda
Contributor

omarocegueda commented Dec 6, 2014

This PR fixes issue #464. This issue was caused by differences in floating-point operations when using double precision (64 bits), extended precision (80 bits) or a combination of both, whcih depends on the platform, compiler and compilation options used.

In summary, these are the proposals implemented in this PR:

  1. We always attempt to compile with SSE2 instruction set when available, so double precision will be used instead of x87, this way we obtain more consistent results across platforms in general, not only the registration module. If SSE2 is not available, the regression tests (only 10 test cases) are skipped .
  2. We modified the interpolation methods to make them more stable when attempting to interpolate beyond the image boundaries. Here is a discussion related to this: scipy/scipy#4075.
  3. In addition to adding the user-defined DEF's for optional compilation in the .pxi file, we write the same defs to dipy/config.py file to have access to that information from python. These defs are used in the tests to select the appropriate "oracle" energy profile.
@@ -1452,8 +1452,11 @@ def _iterate(self):
#set zero displacements at the boundary
fw_step[0, ...] = 0
fw_step[:, 0, ...] = 0
fw_step[-1, ...] = 0

This comment has been minimized.

@omarocegueda

omarocegueda Dec 6, 2014

Contributor

We need to set to zero all the boundaries. I missed the other side!

@@ -11,6 +11,8 @@ def test_cc_factors_2d():
"""
a = np.array(range(20*20), dtype=floating).reshape(20,20)
b = np.array(range(20*20)[::-1], dtype=floating).reshape(20,20)
a /= a.max()

This comment has been minimized.

@omarocegueda

omarocegueda Dec 6, 2014

Contributor

This is to keep the values in a manageable range (not too large) so we don't get different results in the first decimals when the processor is using 64 bits or 80 bits

@@ -12,6 +13,9 @@
import nibabel.eulerangles as eulerangles
from dipy.align.imwarp import DiffeomorphicMap
from dipy.align import VerbosityLevels
from dipy.__config__ import USING_VC_SSE2, USING_GCC_SSE2
NO_SSE2 = not (USING_VC_SSE2 or USING_GCC_SSE2)

This comment has been minimized.

@omarocegueda

omarocegueda Dec 6, 2014

Contributor

If the SSE2 flags were not accepted during compilation, we will skip the regression tests.

assert_equal(inside[i], 0)
else:
assert_equal(inside[i], 1)
#Test interpolation stability along the boundary

This comment has been minimized.

@omarocegueda

omarocegueda Dec 6, 2014

Contributor

I added this test to verify that small differences in the interpolation coordinates near the boundary produce small differences in the interpolation result (i.e. the new interpolation is more stable along the boundary)

energy_profile = subsample_profile(
optimizer.full_energy_profile, 10)
print(energy_profile)

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 8, 2014

Member

pep 8 remove empty line

energy_profile = subsample_profile(
optimizer.full_energy_profile, 10)
print(energy_profile)

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 8, 2014

Member

pep 8 remove empty line

energy_profile = subsample_profile(
optimizer.full_energy_profile, 10)
print(energy_profile)

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 8, 2014

Member

pep 8 remove empty line

energy_profile = subsample_profile(
optimizer.full_energy_profile, 10)
print(energy_profile)

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 8, 2014

Member

pep 8 remove empty line

subsampled_energy_profile = subsample_profile(
optimizer.full_energy_profile, 10)
print(subsampled_energy_profile)

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 8, 2014

Member

pep8: one empty line between code blocks inside functions

subsampled_energy_profile = subsample_profile(
optimizer.full_energy_profile, 10)
print(subsampled_energy_profile)

This comment has been minimized.

@Garyfallidis
energy_profile = subsample_profile(
optimizer.full_energy_profile, 10)
print(energy_profile)

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 8, 2014

Member

as above

energy_profile = subsample_profile(
optimizer.full_energy_profile, 10)
print(energy_profile)

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 8, 2014

Member

as above

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 8, 2014

Hi @omarocegueda this is really great work that you did here with the SSE and the boundary interpolation. Great also to be able know compiler options both from cython and python. This is a game changer for Dipy. Please correct the minor pep8 issues and will go ahead and merge your PR. Thx brother!!! This is really cool!

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 8, 2014

Thank you Elef!,
yes, Matthew's extension to know compiler options from python is very helpful! =). I just made the corrections, let me see if it requires a rebase...

@omarocegueda omarocegueda force-pushed the omarocegueda:fix_profiles branch from 6c199e6 to 24dc623 Dec 8, 2014

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 8, 2014

Rebased =)

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 8, 2014

Travis happy, let's hear the bots! Thx man!

Garyfallidis added a commit that referenced this pull request Dec 8, 2014

Merge pull request #489 from omarocegueda/fix_profiles
Fix diffeomorphic registration test failures

@Garyfallidis Garyfallidis merged commit ec798d7 into nipy:master Dec 8, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment