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

Relax regression tests #806

Merged
merged 4 commits into from Dec 21, 2015

Conversation

Projects
None yet
4 participants
@omarocegueda
Contributor

omarocegueda commented Dec 10, 2015

This is a proposal for relaxing the regression tests of the registration module.
The original idea about the regression tests was to detect subtle changes in the registration module which could affect the overall registration accuracy. Current tests attempt to reproduce the evolution of the energy profile along the full optimization, this is a very strict test because it basically means attempting to perform exactly the same sequence of computations with the same precision. However, we have been facing a lot of problems reproducing the full energy profile across platforms, especially in 64 bits windows.

Instead of requiring the full sequence of computations being the same, this change only measures the final registration result, so the optimization may take different paths depending on the platform, but the final result should be of good quality.

@arokem

This comment has been minimized.

Member

arokem commented Dec 10, 2015

Another option is to run this on other platforms as well, and save the different energy trajectories, as well as the final results. Would that work? If we do decide to go with these changes, you would also need to at least change the docstrings of the functions to explain what they are checking now (e.g. for test_ssd_2d_gauss_newton).

@arokem

This comment has been minimized.

Member

arokem commented Dec 10, 2015

Also, have you run a try_branch with this on the bots that were having issues with these tests?

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 10, 2015

That's right, we may add a different profile for each case, but I'm afraid that's not going to be scalable. This guy summarizes very well the difficulties of trying to reproduce the results of floating point operations in different platforms:

https://randomascii.wordpress.com/2012/03/21/intermediate-floating-point-precision/

So the result of the floating point operations depends on too many factors, including of course the architecture, the compiler version, and the particular optimizations and flags we specify.

After a long conversation with @matthew-brett , we finally achieved reproducibility assuming SSE2 instructions were available (in summary, by asking the compiler to use SSE2 instructions, we were forcing intermediate operations to be performed with 64 bits and not 80 bits). This worked very well in Linux, but MSVC makes things particularly problematic, they textually say:
https://msdn.microsoft.com/en-us/library/7t5yh4fd.aspx

The optimizer chooses when and how to use the SSE and SSE2 instructions when /arch is specified. It uses SSE and SSE2 instructions for some scalar floating-point computations when it determines that it is faster to use the SSE/SSE2 instructions and registers instead of the x87 floating-point register stack. As a result, your code may actually use a mixture of both x87 and SSE/SSE2 for floating-point computations. Also, with /arch:SSE2, SSE2 instructions can be used for some 64-bit integer operations.

The crucial word here is "may" =(. So, it is unpredictable and we ended up adding a new profile for windows. My concern is that it may take a considerable amount of time to update the profiles each time we add a new buildbot and/or each time we want to make an enhancement to the algorithms and it may unnecessarily delay the releases.

Having said that, this is just a proposal, if you guys thing it is worth the effort adding the extra profiles, I can go ahead and update them. Please let me know what you think.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 10, 2015

The regression tests pass on dipy-bdist64-35-win10 after relaxing:

http://nipy.bic.berkeley.edu/builders/dipy-bdist64-35-win10/builds/1/steps/shell_11/logs/stdio

I will try on the other two when they are back online.

@arokem

This comment has been minimized.

Member

arokem commented Dec 10, 2015

Is this branch up-to-date with current master? There are some TripWire
errors in there that I thought we already fixed here:

#801

But it might also be something else...

On Wed, Dec 9, 2015 at 9:06 PM, Omar Ocegueda notifications@github.com
wrote:

The regression tests pass on dipy-bdist64-35-win10 after relaxing:

http://nipy.bic.berkeley.edu/builders/dipy-bdist64-35-win10/builds/1/steps/shell_11/logs/stdio

I will try on the other two when they are back online.


Reply to this email directly or view it on GitHub
#806 (comment).

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 11, 2015

Yes, I re-based the branch just before doing the pull request (and I doube-check right now). The other 64-bit buildbots are back online, I'll run try_branch now.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 11, 2015

I just updated the docstrings.

These tests pass on dipy-bdist64-35 too:
http://nipy.bic.berkeley.edu/builders/dipy-bdist64-35/builds/6/steps/shell_11/logs/stdio

but it still reports the Tripwire error.

@omarocegueda omarocegueda changed the title from WIP: relax regression tests to Relax regression tests Dec 11, 2015

@arokem

This comment has been minimized.

Member

arokem commented Dec 11, 2015

That's weird. Many of these errors do not appear on Windows 8, which is what is currently running on Appveyor: https://ci.appveyor.com/project/arokem/dipy/build/1.0.508/job/4kn2da2a15boeprd

@arokem

This comment has been minimized.

Member

arokem commented Dec 11, 2015

But I have zero understanding of the differences between these systems, so I am not sure how to proceed. I did spend a few hours yesterday trying to build dipy on an AWS Windows 7 machine. Unpleasant.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 11, 2015

Is it possible the extra errors are caused by installing dipy as a wheel rather than python setup.py install ?

@arokem

This comment has been minimized.

Member

arokem commented Dec 11, 2015

That makes sense. Where/how is the wheel dipy-0.11.0.dev0-cp35-none-win_amd64.whl generated?

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 11, 2015

You can follow along the build steps in - say - http://nipy.bic.berkeley.edu/builders/dipy-bdist64-27/builds/5

The wheel gets made with python setup.py bdist_wheel.

@arokem

This comment has been minimized.

Member

arokem commented Dec 11, 2015

What shell is being used here? In the Windows installation I have access to
there are now (after installing VS 2015), at least three: CMD, PowerShell,
and the MSBuild Command Prompt for VS2015.

On Fri, Dec 11, 2015 at 10:25 AM, Matthew Brett notifications@github.com
wrote:

You can follow along the build steps in - say -
http://nipy.bic.berkeley.edu/builders/dipy-bdist64-27/builds/5

The wheel gets made with python setup.py bdist_wheel.


Reply to this email directly or view it on GitHub
#806 (comment).

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 11, 2015

cmd I think - but is that relevant?

@arokem

This comment has been minimized.

Member

arokem commented Dec 12, 2015

I don't know. Is it?

On Fri, Dec 11, 2015 at 11:38 AM, Matthew Brett notifications@github.com
wrote:

cmd I think - but is that relevant?


Reply to this email directly or view it on GitHub
#806 (comment).

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 12, 2015

I don't think it is relevant, but I'm happy to be corrected.

Parameters
----------
image: 2d array shape(r, c)

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 13, 2015

Member

possible docstring error add space after image

This comment has been minimized.

@omarocegueda

omarocegueda Dec 14, 2015

Contributor

Added

----------
image: 2d array shape(r, c)
the image to be deformed
nslices: int

This comment has been minimized.

@Garyfallidis

This comment has been minimized.

@omarocegueda

omarocegueda Dec 14, 2015

Contributor

Added

def test_mult_aff():
r"""mult_aff from imwarp returns the matrix product A.dot(B) considering
None as the identity
r""" Registration: test matrix multiplication using None as identity

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 13, 2015

Member

Please remove Registration: from the title of the dosctrings.

This comment has been minimized.

@omarocegueda

omarocegueda Dec 14, 2015

Contributor

removed

-------
vol : array shape(r, c, `nslices`)
the volumed generated using the undeformed image
wvol :

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 13, 2015

Member

missing dtype

This comment has been minimized.

@omarocegueda

omarocegueda Dec 14, 2015

Contributor

fixed

done by registering the 18 manually annotated T1 brain MRI database IBSR
with each other and computing the jaccard index for all 31 common
anatomical regions.
r''' Registration: test 3D SyN with SSD metric, Gauss-Newton optimizer

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 13, 2015

Member

Same as before please remove Registration: from all tests. You are in the alignment module so these tests are of course about registration.

This comment has been minimized.

@omarocegueda

omarocegueda Dec 14, 2015

Contributor

Removed

if __name__=='__main__':
test_scale_space_exceptions()
test_optimizer_exceptions()
test_mult_aff()
test_diffeomorphic_map_2d()

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 13, 2015

Member

Please replace calling all the functions in the main and just use
run_module_suite and that will call all available tests in the file. Import this function from ``numpy.testing`.

This comment has been minimized.

@omarocegueda

omarocegueda Dec 14, 2015

Contributor

removed

@arokem

This comment has been minimized.

Member

arokem commented Dec 16, 2015

The TripWire error should now be fixed with the recent merge of #808.

On Thu, Dec 10, 2015 at 6:33 PM, Omar Ocegueda notifications@github.com
wrote:

I just updated the docstrings.

These tests pass on dipy-bdist64-35 too:

http://nipy.bic.berkeley.edu/builders/dipy-bdist64-35/builds/6/steps/shell_11/logs/stdio

but it still reports the Tripwire error.


Reply to this email directly or view it on GitHub
#806 (comment).

omarocegueda added some commits Dec 10, 2015

@omarocegueda omarocegueda force-pushed the omarocegueda:relax_imwarp_tests branch from b6ef176 to 7afd608 Dec 16, 2015

@omarocegueda

This comment has been minimized.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 16, 2015

@MarcCote can you check Omar's last message on top? One of the errors are from the new clustering framework. Thx!

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 16, 2015

Eleftherios - I guess you mean this error : #813 ?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 18, 2015

Yes, I mean this error @matthew-brett Is this already resolved?

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 19, 2015

Not as far as I know.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 19, 2015

ok

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 21, 2015

Thx @omarocegueda :)

Garyfallidis added a commit that referenced this pull request Dec 21, 2015

@Garyfallidis Garyfallidis merged commit 4cee726 into nipy:master Dec 21, 2015

1 check passed

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