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

Diffeomorphic registration test failures on PPC #464

Closed
matthew-brett opened this Issue Nov 13, 2014 · 10 comments

Comments

Projects
None yet
3 participants
@matthew-brett
Member

matthew-brett commented Nov 13, 2014

See : http://nipy.bic.berkeley.edu/builders/dipy-py2.6-osx-10.5-ppc/builds/358/steps/shell_8/logs/stdio

Omar - this is another platform with different floating point I guess. Do you have any insight into what is going on here? Would you like an account on that machine? You can surely also use the try_branch.py script with builder dipy-py2.6-osx-10.5-ppc.

@arokem

This comment has been minimized.

Member

arokem commented Nov 13, 2014

Wow - you have an actual PPC running 10.5? That's a real relic!

Maybe we should consider dropping support for this platform/OS?

On Thu, Nov 13, 2014 at 11:37 AM, Matthew Brett notifications@github.com
wrote:

See :
http://nipy.bic.berkeley.edu/builders/dipy-py2.6-osx-10.5-ppc/builds/358/steps/shell_8/logs/stdio

Omar - this is another platform with different floating point I guess. Do
you have any insight into what is going on here? Would you like an account
on that machine? You can surely also use the try_branch.py script with
builder dipy-py2.6-osx-10.5-ppc.


Reply to this email directly or view it on GitHub
#464.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Nov 13, 2014

I don't think specific support for this platform is of much interest. It's only interesting to the extent that the platform reveals delicacies in the stability of the algorithm. So, if it would not take more than an hour or so, I think it is worth diagnosing the problem. When we understand the problem, it may well turn out to be safe to skip this test for this platform.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Nov 13, 2014

Hi Matthew!,
Thank you very much for pointing that out!, I forgot to mention my progress
on that regard: I finished adding the extra energy profiles to check
against in win32, the tests now pass in all the platforms I tested (windows
and linux, 32 and 64 bit):
http://nipy.bic.berkeley.edu/builders/dipy-py2.7-win32/builds/92/steps/shell_8/logs/stdio
http://nipy.bic.berkeley.edu/builders/dipy-py2.6-32/builds/394/steps/shell_7/logs/stdio
The new tests already include the new, more stable, interpolation along the
boundary, as we previously discussed (I guess it is ok to have this fix for
now, and if we decide to do something different we can implement it later).

I just have a couple questions:

  1. I added the flag to compile with SSE2 instructions when they're
    available, like this:
    https://github.com/omarocegueda/dipy/blob/fix_boundary/setup.py#L126
    this fixes the differences in the energy profile in linux32. Are we sure
    it is ok to have that flag? (I would vote yes, at least that way the
    computations would be more consistent across platforms, and as far as I
    know it should not hurt performance)
  2. I needed to somehow expose the flags from dipy/build/config.pxi to
    python. The way I did it was to define a function in vector_fields.pxy
    like this:
    https://github.com/omarocegueda/dipy/blob/fix_boundary/dipy/align/vector_fields.pyx#L15
    this is probably not the best place to put this. This distribution
    properties may be useful for someone else, maybe add a
    dipy/build/config.py that exports the same flags as the .pxi to be used
    in python? I can implement that, I would just like to know if it is a good
    idea to do that, what do you think?.

Meanwhile, let me try_branch on the dipy-py2.6-osx-10.5 buildbot to see if
we still get differences.

Thanks!

On Thu, Nov 13, 2014 at 1:44 PM, Matthew Brett notifications@github.com
wrote:

I don't think specific support for this platform is of much interest. It's
only interesting to the extent that the platform reveals delicacies in the
stability of the algorithm. So, if it would not take more than an hour or
so, I think it is worth diagnosing the problem. When we understand the
problem, it may well turn out to be safe to skip this test for this
platform.


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

"Cada quien es dueño de lo que calla y esclavo de lo que dice"
-Proverbio chino.
"We all are owners of what we keep silent and slaves of what we say"
-Chinese proverb.

http://www.cimat.mx/~omar

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Nov 13, 2014

Congratulations on getting the windows tests to pass, that's very good.

Yes, I think we can depend on SSE2 at this stage. OSX requires SSE2, and my analysis of Firefox crash reports puts SSE2 on Windows at above 99% : https://gist.github.com/matthew-brett/9cb5274f7451a3eb8fc0 I'm hoping that dipy users will have better than average hardware, so likely even fewer non-SSE2 machines. I don't think it should hurt performance, compared to x87.

Maybe it would be worth having a look at the numpy __config__.py mechanism? Numpy writes this file when building with parameters from the build.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Nov 29, 2014

Hi Matthew,
sorry for being so slow, I was studying how the config.py is generated in numpy, and from what I understand (not much, I have to admit), in numpy, they organize the information exposed in config.py as a set of dictionaries, each dictionary is associated to a class derived from system_info:

https://github.com/numpy/numpy/blob/master/numpy/distutils/system_info.py#L437

To add a new dictionary that will be exposed in config.py, they create a class derived from system_info:

https://github.com/numpy/numpy/blob/master/numpy/distutils/system_info.py#L750

and add entries by calling the set_info method:

https://github.com/numpy/numpy/blob/master/numpy/distutils/system_info.py#L785

the base class then adds a new entry to a static dictionary where the key is a string equal to the name of the derived class, and the value is a dictionary containing the information for that derived class:

https://github.com/numpy/numpy/blob/master/numpy/distutils/system_info.py#L490

Here I wrote a simplified class implementing that for dipy:

https://github.com/omarocegueda/dipy/blob/fix_profiles/setup_helpers.py#L35

Of course, this is much simpler than what numpy does, but maybe it's enough for our current needs (expose the custom flags generated by the Checker to be used in python, not only in cython). The problem I am having right now is that the generated dipy/config.py is not copied to the installation directory, and then the config.py module is not found:

http://nipy.bic.berkeley.edu/builders/dipy-bdist32-27/builds/151/steps/shell_8/logs/stdio

By any chance do you know how can I include the generated config.py file to the list of files that must be copied to the destination install directory?

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 1, 2014

After several attempts, I couldn't make the --config--.py to be copied to the module installation directory, the problem is that all the python files are copied to the build directory before the cython extensions are generated. So, instead of generating a --config--.py, I generated a --config--.pyx and added it as another extension:
https://github.com/omarocegueda/dipy/blob/fix_profiles/setup.py#L97

I know this may be too "hacky", but at least I could run the fixes to the registration tests on the buildbots that were failing. These were the problematic buildbots:

dipy-py2.6-osx-10.5-ppc: Now it passes:
http://nipy.bic.berkeley.edu/builders/dipy-py2.6-osx-10.5-ppc

dipy-bdist32-33: Now it skips the tests (apparently SSE2 is not supported, the compiler flags were not recognized):
http://nipy.bic.berkeley.edu/builders/dipy-bdist32-33/builds/59/steps/shell_8/logs/stdio

dipy-py2.6-32: Now it passes:
http://nipy.bic.berkeley.edu/builders/dipy-py2.6-32

dipy-py2.7-win32: Now it passes (the failure is not related to the registration module)
http://nipy.bic.berkeley.edu/builders/dipy-py2.7-win32/builds/116/steps/shell_6/logs/stdio

EDIT: sorry, dipy-bdist32-27 is still failing:
http://nipy.bic.berkeley.edu/builders/dipy-bdist32-27/builds/154/steps/shell_8/logs/stdio
I am investigating...
EDIT2: sorry again, it's ok, the failure is not related to diffeomorphic registration.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 4, 2014

Sorry - maybe that wasn't a good idea trying to follow the __config__.py
idea in numpy; I believe the numpy distutils code is rather complicated.

What about the very crude idea I just posted in my config-py-idea branch?

It just defines the same variables in the cython pxi file, nothing fancy.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 5, 2014

Hi Matthew!,
I just used your implementation and it worked perfectly. I was writing the config.py file too, but it was not being copied to the module installation directory, now I see the field "self.build_lib", that's exactly what I needed =).

I didn't want to put the code to generate the file inside the Checker's build_extensions method to allow other developers add their own config fields in the future. However, right now I cannot imagine why people would need more fields besides the compilation flags, so I'm ready to make a PR with my corrections to the tests on top of your config.py implementation. Do you agree? or do you think it is better to do two separate PRs (one for the config, and one for my tests)?

Thanks!

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 5, 2014

On Thursday, December 4, 2014, Omar Ocegueda notifications@github.com
wrote:

Hi Matthew!,
I just used your implementation and it worked perfectly. I was writing the
config.py file too, but it was not being copied to the module
installation directory, now I see the field "self.build_lib", that's
exactly what I needed =).

I didn't want to put the code to generate the file inside the Checker's
build_extensions method to allow other developers add their own config
fields in the future. However, right now I cannot imagine why people would
need more fields besides the compilation flags, so I'm ready to make a PR
with my corrections to the tests on top of your config.py
implementation. Do you agree? or do you think it is better to do two
separate PRs (one for the config, and one for my tests)?

I think that's fine - we can extend the config code later if people find
they need it, but you may well be right, that that won't happen.

One PR is fine I think - go for it.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 30, 2014

I think this one closed by #489 - buildbots current passing on PPC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment