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

Fixes issue #813 by not checking data type explicitly. #829

Merged
merged 4 commits into from Jan 12, 2016

Conversation

Projects
None yet
4 participants
@MarcCote
Contributor

MarcCote commented Jan 7, 2016

According to issue #813, on Windows 64bit machines, integer constants are of type long instead of int. I added a unit test that explicitly test using a long as the type of the value returned by feature.infer_shape.

This PR also fixes another issue that came up in metric.dist while creating the new unit test.

@arokem

This comment has been minimized.

Member

arokem commented Jan 9, 2016

Well - this fix seems to work... in Python 2.

See: http://python3porting.com/differences.html#long

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jan 9, 2016

Alright, I return 1 or long(1) whether sys.version_info > (3,) or not.

@arokem

This comment has been minimized.

Member

arokem commented Jan 9, 2016

LGTM. Let's wait and see what Travis says. In the meanwhile, I've started builds on the 64 bit Windows machine that was having trouble before:

http://nipy.bic.berkeley.edu/builders/dipy-bdist64-27/builds/21
http://nipy.bic.berkeley.edu/builders/dipy-bdist64-35/builds/23

@arokem

This comment has been minimized.

Member

arokem commented Jan 9, 2016

Yeah - everything's coming back green. Any objections from anyone? I'll wait for a few days for others to take a look and then merge.

@@ -284,6 +285,26 @@ def extract(self, streamline):
d2 = metric.dist(features1, features2)
assert_equal(d1, d2)
class ArcLengthFeature(dipymetric.Feature):
def infer_shape(self, streamline):
if sys.version_info > (3,):

This comment has been minimized.

@matthew-brett

matthew-brett Jan 9, 2016

Member

I find this very ugly. Is there any way of avoiding it?

This comment has been minimized.

@arokem

arokem Jan 9, 2016

Member

Would this do it? MarcCote@1368756

This comment has been minimized.

@matthew-brett

matthew-brett Jan 9, 2016

Member

Less ugly but still hard to understand. Could y'all remind me why this is necessary? Isn't the return type dealt with by https://github.com/nipy/dipy/pull/829/files#diff-6102332b6a9436b5c5b18ab9ee552a9eR169 ?

This comment has been minimized.

@matthew-brett

matthew-brett Jan 9, 2016

Member

Oh - sorry - I see you are trying to test the Windows behavior on non-Windows machines - maybe just a comment to that effect.

This comment has been minimized.

@MarcCote

MarcCote Jan 11, 2016

Contributor

As @matthew-brett said I'm trying to test the Windows 64 bits behavior, i.e. where it uses long instead of int for constant integers. I added a comment explaining why we do such test.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 12, 2016

Fine with me too to merge this.

arokem added a commit that referenced this pull request Jan 12, 2016

Merge pull request #829 from MarcCote/fix_issue813_windows_error
Fixes issue #813 by not checking data type explicitly.

@arokem arokem merged commit 17e8a4d into nipy:master Jan 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcCote MarcCote deleted the MarcCote:fix_issue813_windows_error branch Jan 13, 2016

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