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 buildbots errors introduced with the registration module #443

Merged
merged 7 commits into from Nov 1, 2014

Conversation

Projects
None yet
3 participants
@omarocegueda
Contributor

omarocegueda commented Oct 10, 2014

Most failures are caused because "__isinf" is not defined in Visual C. Here, I am using cimport from libc.math (the library provided by cython) instead of importing directly from math.h.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 11, 2014

@MarcCote , @matthew-brett, @MrBago do you know of any better way to check for inf? Than what Omar is proposing here?

@@ -369,7 +372,7 @@ cpdef double iterate_residual_displacement_field_ssd_3d(
y[0] += disp[ds, dr, dc, 0]
y[1] += disp[ds, dr, dc, 1]
y[2] += disp[ds, dr, dc, 2]
if(isinf(sigmasq)):
if isinf(sigmasq) != 0:

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 11, 2014

Member

I think here by enabling the gil from inside isinf you will lose performance (because you are inside nested for loops).

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 11, 2014

Member

And reasons why sigmasq can become infinite? Can you grab it before that happens?

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Oct 11, 2014

Omar - can you use something like http://stackoverflow.com/a/3275526 in dpy_math.h ?

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Oct 11, 2014

Thank you for your comments! and sorry for using Travis to test different approaches, the problem is that I am unable to reproduce the failure in my linux and windows, I guess it has to do with the cython version (I have 0.19 and 0.20, but even if I try to force 0.18 with conda create, it still installs a later version). With my distribution, I can cimport isinf from libc.math and use it inside a nogil block, but it fails on Travis (isinf was added in a version later than 0.18?). The variance is infinite when there are less than two samples. In this particular case, I can use negative values as flags (-1 for infinite, for example), but I would like to use that as a "desperate" last resource.

Let me try what Matthew suggested, although at first sight it seems like we need some kind of conditional compilation for visual C (I don't know how to do that in cython, let me investigate a bit on the web).

Also, any progress on how to run the buildbots on specific branches? It would be nice to have a way to repro the failures without bothering you guys with pull requests...

EDIT: Oh! and yes, using numpy's isinf is extremely slow, I definitely need to do something else.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Oct 11, 2014

Aha - Eleftherios and I were just discussion conditional compilation. Would something like work:

diff --git a/src/dpy_math.h b/src/dpy_math.h
index defea57..f1d213e 100644
--- a/src/dpy_math.h
+++ b/src/dpy_math.h
@@ -17,3 +17,7 @@ double dpy_log2(double x)
     return NPY_LOG2E*log(x);
 }
 #endif
+
+#ifdef _MSC_VER
+#define isinf(x) (!_finite(x))
+#endif

? I will try to get to setting up custom branch compiles on the buildbots soon, but no promises for today, sorry :(

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Oct 11, 2014

Actually, I think just using dpy_math.h unmodified will work, because it will define isinf in a platform specific way via npy_math.h - as in:

cdef extern from "dpy_math.h" nogil:
    double isinf(double x)

Does that work?

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Oct 12, 2014

Oh! so nice! so that's what dpy_math.h is for... =) it worked on Travis, we just need to check if it does with Visual C (although if it doesn't, now it's clear how to do the conditional compilation). Thanks Matthew!, it's great to know how to do this.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Oct 12, 2014

Omar - can you send me via email your public ssh key so I can give you access to the buildbot machine? I think I have worked out how to let you trigger builds for code that hasn't been committed yet, but I need to test a little bit.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Oct 12, 2014

Sure!,

ssh-rsa
AAAAB3NzaC1yc2EAAAADAQABAAABAQDHi+Ast0fdgTzqzgg1XWksGy2LgZPmEmD5YWhCIrWLTIv8OD49OthC02m7KD0fkgR3IWQZfv/DPUJdQnUwoLD+xif9k0Fo7D5+U+zHYeZiwmvLkcpEZd0Kz6F29WC00a0rBXRAY281ppMRGvrojr1Mt5BDkJBiRP9Q4+NrH6n3PTsRyRCBRaBqyWk5rNghHsWgMcGZTAcFwh8BW9nMAsAluEZuEYG0cxoXoXwXhIfO2tpNUTeI481nJiGmEbF7SRN2TIXbzIo+q0ysBgBw/PtBJcFCkMKHekvnEyxNE+giBlEznpw43SBxbVm/7z4ufd409barFo96Z2PkNRsnQBGD
jomaroceguedag@gmail.com

thanks! =D

On Sun, Oct 12, 2014 at 2:02 AM, Matthew Brett notifications@github.com
wrote:

Omar - can you send me via email your public ssh key so I can give you
access to the buildbot machine? I think I have worked out how to let you
trigger builds for code that hasn't been committed yet, but I need to test
a little bit.


Reply to this email directly or view it on GitHub
#443 (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

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 26, 2014

Hi guys, sorry I lost track with this PR. Is this now ready to be merged? Some feedback please. Thx...

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Oct 26, 2014

This already fixes the build errors on win32, but 10 tests fail on 32-bit
platforms (windows and linux). I am investigating, but it seems the
regression tests won't work across architectures. I'm not sure if it is ok
to merge this now and do another PR for the tests, what do you think?

On Sun, Oct 26, 2014 at 10:28 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Hi guys, sorry I lost track with this PR. Is this now ready to be merged?
Some feedback please. Thx...


Reply to this email directly or view it on GitHub
#443 (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 1, 2014

I think it is OK to merge this now, and do another PR for the tests.

The branch does need a rebase though.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Nov 1, 2014

I just rebased to master

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Nov 1, 2014

The history looks a little weird here - there are two copies of the commits, one merged and the other rebased, as far as I can tell. e.g. 1547da1 and 442da53 . Can you clean that up? Can I help?

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Nov 1, 2014

Yes, you are right, I don't know why it duplicated the commits. I don't know how to fix this kind of things though, except I can close this PR and make a new one wit only one commit, is that acceptable?

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Nov 1, 2014

There seems to be no difference between commit f6d5627 (your current branch position, with both copies of the commits) and 442da53 (the tip of one set of the commits, based on current master).

So I think you can just do:

git reset --hard 442da53
git push --force

on this branch, to get the correct history, and the same working tree as your current branch.

@omarocegueda omarocegueda force-pushed the omarocegueda:fix_isinf branch from f6d5627 to 442da53 Nov 1, 2014

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Nov 1, 2014

Done, thanks! =)

matthew-brett added a commit that referenced this pull request Nov 1, 2014

Merge pull request #443 from omarocegueda/fix_isinf
MRG: Fix buildbots errors introduced with the registration module

Most failures are caused because "__isinf" is not defined in Visual C. Use imports from `dpy_math` instead.

Remove dependency on matplotlib by using `npy` files for data instead of `png` files.

@matthew-brett matthew-brett merged commit f03490e into nipy:master Nov 1, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Nov 1, 2014

Thanks a lot for this - it's hard work doing this cross-platform stuff.

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