Skip to content

[MRG + 2] FIX precision to float64 across the codebase #5375

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

Merged
merged 1 commit into from
Oct 19, 2015

Conversation

raghavrv
Copy link
Member

@raghavrv raghavrv commented Oct 9, 2015

Implementing #5356 (comment)

@ogrisel @giorgiop Does this look okay?

@raghavrv raghavrv force-pushed the set_precision branch 2 times, most recently from 7a6f80c to 37794ea Compare October 9, 2015 13:22
@amueller
Copy link
Member

amueller commented Oct 9, 2015

Did you just replace all float32 with float64? Some parts of sklearn work in float64, right? Or not any more? The trees do and the SVM does IIRC. So in some places we want float32.

@raghavrv
Copy link
Member Author

I've changed

  • all float to float64.
  • all float32 in some tests to float64 (under the assumption that if it passes in float64 it should pass for float32 too right?)
  • all float32 in non test code is left as such.

Are these the proper things to do??

],
'continuous-multioutput': [
np.array([[0, .5], [.5, 0]]),
np.array([[0, .5], [.5, 0]], dtype=np.float32),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests that checks for np.float32 should be left unchanged.

@ogrisel
Copy link
Member

ogrisel commented Oct 12, 2015

all float32 in some tests to float64 (under the assumption that if it passes in float64 it should pass for float32 too right?)

No this is a wrong assumption. 32 bit is less precise than 64 bit and some our functions and class methods might use the dtype of the input to select the precision level of their internal datastructures. Please leave the np.float32 tests unchanged.

@raghavrv
Copy link
Member Author

Thanks for clarifying that!! I'll revert in a minute :)

@ogrisel
Copy link
Member

ogrisel commented Oct 12, 2015

Note that as far as I know np.dtype(np.float) always yields np.float64 on all our supported architectures (even 32 bit Python), but it's more explicit to use np.float64 directly in our code instead of relying of potentially platform specific behaviors.

@raghavrv raghavrv force-pushed the set_precision branch 2 times, most recently from 31d1de0 to 1cc1dee Compare October 12, 2015 11:22
@raghavrv
Copy link
Member Author

@ogrisel Have reverted all float32 --> float64 changes in tests... Could you give this a go now?

@@ -1451,7 +1451,7 @@ class OneHotEncoder(BaseEstimator, TransformerMixin):
>>> enc = OneHotEncoder()
>>> enc.fit([[0, 0, 3], [1, 1, 0], [0, 2, 1], \
[1, 0, 2]]) # doctest: +ELLIPSIS
OneHotEncoder(categorical_features='all', dtype=<... 'float'>,
OneHotEncoder(categorical_features='all', dtype=<... 'numpy.float64'>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait so the type is the same but the string representation is different? that is somewhat confusing, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as my understanding goes... numpy.float is alias of python's default float while numpy.float64 is a scalar type defined by numpy...
(This answer seems to explain it... (especially the comment below it!))

@amueller
Copy link
Member

lgtm

@amueller amueller changed the title [MRG] FIX precision to float64 across the codebase [MRG + 1] FIX precision to float64 across the codebase Oct 12, 2015
@raghavrv
Copy link
Member Author

@ogrisel could you take a look? :)

@glouppe
Copy link
Contributor

glouppe commented Oct 19, 2015

Looks good to me too. +1 for merge.

@glouppe glouppe changed the title [MRG + 1] FIX precision to float64 across the codebase [MRG + 2] FIX precision to float64 across the codebase Oct 19, 2015
glouppe added a commit that referenced this pull request Oct 19, 2015
[MRG + 2] FIX precision to float64 across the codebase
@glouppe glouppe merged commit 008adf1 into scikit-learn:master Oct 19, 2015
@raghavrv
Copy link
Member Author

@glouppe Thanks :)

@raghavrv raghavrv deleted the set_precision branch October 19, 2015 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants