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 round #494

Merged
merged 6 commits into from Dec 14, 2014

Conversation

Projects
None yet
4 participants
@omarocegueda
Contributor

omarocegueda commented Dec 9, 2014

This is a follow-up to #493.

Apparently, round is not defined in c++98:
http://stackoverflow.com/questions/485525/round-for-float-in-c

which causes this error:
http://nipy.bic.berkeley.edu/builders/dipy-py2.7-win32/builds/124/steps/shell_5/logs/stdio

Using floor(x+0.5) may not be consistent with numpy, we also need to consider rounding to nearest even:
http://mathematica.stackexchange.com/questions/2116/why-round-to-even-integers

This solution uses the same procedure as for "dpy_log2", by borrowing code from numpy:
https://github.com/numpy/numpy/blob/master/numpy/core/src/npymath/npy_math.c.src

The code now builds in buildbot dipy-py2.7-win32:
http://nipy.bic.berkeley.edu/builders/dipy-py2.7-win32/builds/127/steps/shell_5/logs/stdio

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 9, 2014

#endif
/* From numpy npy_math.c.src */
#ifndef HAVE_RINT

This comment has been minimized.

@MrBago

MrBago Dec 9, 2014

Contributor

This looks good, out of curiosity when do we expect npy_math to not have npy_rint?

This comment has been minimized.

@omarocegueda

omarocegueda Dec 10, 2014

Contributor

Honestly, I don't know, I may have been too "paranoid", like the codility.com guys say: "Assume nothing, trust no one" ;-) . Maybe it's not necessary, I'll check as soon as the buildbots get back online =)

@@ -7,7 +7,7 @@
#include "numpy/npy_math.h"
#define dpy_isnan npy_isnan
#define dpy_log2 npy_log2
#define dpy_signbit npy_signbit

This comment has been minimized.

@MrBago

MrBago Dec 9, 2014

Contributor

Can we just use the names npy_isnan and npy_signbit? Do we need to rename them? Is there a case where we expect to override these numpy functions?

This comment has been minimized.

@matthew-brett

matthew-brett Dec 9, 2014

Member

On Tuesday, December 9, 2014, MrBago notifications@github.com wrote:

In src/dpy_math.h
#494 (diff):

@@ -7,7 +7,7 @@
#include "numpy/npy_math.h"

#define dpy_isnan npy_isnan
-#define dpy_log2 npy_log2
+#define dpy_signbit npy_signbit

Can we just use the names npy_isnan and npy_signbit? Do we need to rename
them? Is there a case where we expect to override these numpy functions?

Yes, I think you can use the original names.

This comment has been minimized.

@omarocegueda

omarocegueda Dec 10, 2014

Contributor

Ok!, in general, I don't see why we would need to override these basic numpy functions. I can do the changes to use npy_rint and npy_log2 as well, do you guys agree?

This comment has been minimized.

@MrBago

MrBago Dec 10, 2014

Contributor

Sounds good, thanx Omar.
On Dec 10, 2014 10:23 AM, "Omar Ocegueda" notifications@github.com wrote:

In src/dpy_math.h
#494 (diff):

@@ -7,7 +7,7 @@
#include "numpy/npy_math.h"

#define dpy_isnan npy_isnan
-#define dpy_log2 npy_log2
+#define dpy_signbit npy_signbit

Ok!, in general, I don't see why we would need to override these basic
numpy functions. I can do the changes to use npy_rint and npy_log2 as well,
do you guys agree?


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/494/files#r21623868.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 11, 2014

I was reading a bit more the numpy code and I think the npy_* versions are always available. If the native version is available, they simply decorate them with prefix 'npy_':

https://github.com/numpy/numpy/blob/a435de365a603f29bf243882644f1e26edda6e9a/numpy/core/src/npymath/npy_math.c.src#L433
here is where they call the native function:
https://github.com/numpy/numpy/blob/a435de365a603f29bf243882644f1e26edda6e9a/numpy/core/src/npymath/npy_math.c.src#L450

and if the native function is not available, then they define different versions of their own implementation with different types of arguments and return values:

https://github.com/numpy/numpy/blob/a435de365a603f29bf243882644f1e26edda6e9a/numpy/core/src/npymath/npy_math.c.src#L362

here is where they call the (generic) version for each return/parameter type:
https://github.com/numpy/numpy/blob/a435de365a603f29bf243882644f1e26edda6e9a/numpy/core/src/npymath/npy_math.c.src#L375

so, in theory, it should be possible to simply call the npy_* version without adding any extra code in dpy_math.h (just including "numpy/npy_math.h" should be enough). Unfortunately the win32 buildbots are still offline... I'll try this as soon as they are back online.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 11, 2014

Not sure what is happening with the buildbots, but I suspect there was a
power failure, and only some of them came back up.

There's a 'storm' at the moment in Berkeley apparently. Thomas Kluyver
works in the next-door office, is at home because of the storm, but if he
goes in, he will reboot the machines. That might not be today though -
sorry to say.

@omarocegueda omarocegueda force-pushed the omarocegueda:fix_round branch from 3571a29 to 771b611 Dec 13, 2014

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 13, 2014

Hi!, I hope you guys are fine after the big storm!. The buildbots are working now, and we effectively can just include "numpy/npy_math.h" from "dpy_math.h" and use all the "npy_*" functions without adding any extra code. After that, the tests pass in win32:
http://nipy.bic.berkeley.edu/builders/dipy-bdist32-27/builds/160/steps/shell_8/logs/stdio
and I had no issues locally, I'm just waiting to see what Travis says...

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 14, 2014

Nice! Thx!

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

@Garyfallidis Garyfallidis merged commit eac86f2 into nipy:master Dec 14, 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