Skip to content

Warn if default numpy.int is 32bits, while running tests#14167

Merged
rth merged 5 commits intoscikit-learn:masterfrom
Freshia:confestUpdate
Jul 2, 2019
Merged

Warn if default numpy.int is 32bits, while running tests#14167
rth merged 5 commits intoscikit-learn:masterfrom
Freshia:confestUpdate

Conversation

@Freshia
Copy link
Copy Markdown
Contributor

@Freshia Freshia commented Jun 23, 2019

Fixes #14029

The doctests would fail if the default numpy.int is 32 bits long. This warns the user if their numpy int is a 32 bit
Contributions made by @adrinjalali and @felexkemboi
#wimlds

Freshia added 2 commits June 23, 2019 03:43
The doctests would fail if the default numpy.int is 32 bits long. This warns the user if their numpy int is a 32 bit
@adrinjalali adrinjalali changed the title Fixes issue #14029 Warn if default numpy.int is 32bits Jun 23, 2019
@adrinjalali adrinjalali changed the title Warn if default numpy.int is 32bits Warn if default numpy.int is 32bits, while running tests Jun 23, 2019
@adrinjalali
Copy link
Copy Markdown
Member

I'm not sure why you've replace all the content of the file.

The issue also asks for a better documentation, which you'd need to include in the PR.

Also, the linter is failing :) (you can check PEP8 for python style guides)

@adrinjalali
Copy link
Copy Markdown
Member

Is this supposed to replace #14163?

@rth
Copy link
Copy Markdown
Member

rth commented Jun 23, 2019

Is this supposed to replace #14163?

Yes, it looks like you were both working on this same issue. Also, my comments in #14163 also apply here as well.

@glemaitre
Copy link
Copy Markdown
Member

I think that @rth as more this in head:

diff --git a/conftest.py b/conftest.py
index f8ced99fc..73326d6d2 100644
--- a/conftest.py
+++ b/conftest.py
@@ -8,10 +8,12 @@
 import platform
 from distutils.version import LooseVersion
 
-from sklearn import set_config
 import pytest
 from _pytest.doctest import DoctestItem
 
+from sklearn import set_config
+from sklearn.utils import _IS_32BIT
+
 PYTEST_MIN_VERSION = '3.3.0'
 
 if LooseVersion(pytest.__version__) < PYTEST_MIN_VERSION:
@@ -51,13 +53,17 @@ def pytest_collection_modifyitems(config, items):
     try:
         import numpy as np
         if LooseVersion(np.__version__) < LooseVersion('1.14'):
+            reason = 'doctests are only run for numpy >= 1.14'
+            skip_doctests = True
+        elif _IS_32BIT:
+            reason = ('doctest are only run when the default numpy int is '
+                      '64 bits.')
             skip_doctests = True
     except ImportError:
         pass
 
     if skip_doctests:
-        skip_marker = pytest.mark.skip(
-            reason='doctests are only run for numpy >= 1.14 and python >= 3')
+        skip_marker = pytest.mark.skip(reason=reason)
 
         for item in items:
             if isinstance(item, DoctestItem):

@Freshia
Copy link
Copy Markdown
Contributor Author

Freshia commented Jun 26, 2019

Thanks @adrinjalali ,I'll work on that

@glemaitre glemaitre self-requested a review July 2, 2019 12:36
Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks!

@rth rth merged commit 03c39af into scikit-learn:master Jul 2, 2019
@rth rth mentioned this pull request Jul 2, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Warn if default numpy.int is 32 bit, or document it.

5 participants