BUG: raise for invalid dtypes per issue #15520 #16047

analyticalmonk · 2017-04-18T12:19:21Z

closes #xxxx
tests added / passed
passes git diff upstream/master --name-only -- '*.py' | flake8 --diff
whatsnew entry

jreback · 2017-04-18T12:42:54Z

pandas/core/generic.py

@@ -165,12 +166,15 @@ def _validate_dtype(self, dtype):

        if dtype is not None:
            dtype = _coerce_to_dtype(dtype)


I think can just directly use pandas._dtype here. In fact I think you can replace all usages of _coerce_to_dtype with pandas_dtype. (you might have to back off some if the tests don't pass), but can investigate those.

Using pandas_dtype() here will mean that [1] will throw an error. This has been used across various tests, and all of them will break. For example, a number of tests here.

[1] pd.Series(values, dtype=object)

not sure what you mean. This is a fairly generic routine.

In [1]: pandas.core.dtypes.common.pandas_dtype(object) Out[1]: dtype('O') In [2]: pandas.core.dtypes.common.pandas_dtype('object') Out[2]: dtype('O')

jreback · 2017-04-18T12:43:26Z

pandas/tests/dtypes/test_common.py

@@ -13,6 +14,15 @@

 class TestPandasDtype(tm.TestCase):

+    # Passing invalid dtype, both as a string or object, must raise TypeError


use a list for checking these cases

if you want to convert to using more pytest style would be graet as well (could be a followup as well), e.g.: http://pandas-docs.github.io/pandas-docs-travis/contributing.html#transitioning-to-pytest

jreback · 2017-04-18T14:14:58Z

pandas/core/dtypes/common.py

@@ -737,5 +737,7 @@ def pandas_dtype(dtype):
            pass
    elif isinstance(dtype, ExtensionDtype):
        return dtype
+    elif np.dtype(dtype).kind == 'O':


this needs to be more defensive

try: dtype = np.dtype(dtype) except (TypeError, ValueError): raise informative msg here if dtype.kind == 'O': .... return dtype

np.dtype doesn't differentiate between [1] and [2]. Any thoughts on how to raise errors for [1] without breaking [2]? Since the change I made to pandas_dtype would raise an error in both cases (pandas_dtype(pd.Timestamp) and pandas_dtype(dtype='object')).

[1]

>>> np.dtype(pd.Timestamp) dtype('O')

[2]

>>> np.dtype('object') dtype('O')

let's add a few more tests cases, FYI (e.g. np.float64, float64, np.dtype('float64'), IOW some objects and strings

maybe something like

if isinstance(dtype, (np.dtype, compat.string_types)): return np.dtype(dtype) raise TypeError

analyticalmonk · 2017-04-20T13:00:15Z

@jreback Updated tests as well as pandas_dtype() function as per your suggestions.

codecov · 2017-04-21T14:01:10Z

Codecov Report

Merging #16047 into master will decrease coverage by <.01%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master   #16047      +/-   ##
==========================================
- Coverage   90.84%   90.84%   -0.01%     
==========================================
  Files         159      159              
  Lines       50790    50794       +4     
==========================================
+ Hits        46142    46143       +1     
- Misses       4648     4651       +3

Flag	Coverage Δ
#multiple	`88.62% <80%> (-0.01%)`	⬇️
#single	`40.35% <60%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/dtypes/cast.py	`86.89% <ø> (ø)`	⬆️
pandas/core/series.py	`94.97% <ø> (ø)`	⬆️
pandas/core/generic.py	`91.29% <100%> (ø)`	⬆️
pandas/core/dtypes/common.py	`92.76% <75%> (-0.87%)`	⬇️
pandas/io/formats/format.py	`95.02% <0%> (-0.03%)`	⬇️
pandas/core/common.py	`91.03% <0%> (+0.34%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f16ca7...1f553e2. Read the comment docs.

codecov · 2017-04-21T14:01:15Z

Codecov Report

Merging #16047 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16047      +/-   ##
==========================================
- Coverage   90.86%   90.83%   -0.03%     
==========================================
  Files         159      159              
  Lines       50771    50779       +8     
==========================================
- Hits        46133    46127       -6     
- Misses       4638     4652      +14

Flag	Coverage Δ
#multiple	`88.61% <100%> (-0.03%)`	⬇️
#single	`40.32% <66.66%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/dtypes/common.py	`93.24% <100%> (-0.39%)`	⬇️
pandas/core/generic.py	`91.29% <100%> (ø)`	⬆️
pandas/core/series.py	`94.98% <100%> (ø)`	⬆️
pandas/core/dtypes/cast.py	`86.89% <100%> (ø)`	⬆️
pandas/plotting/_converter.py	`63.54% <0%> (-1.82%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d122e6...3646eb6. Read the comment docs.

analyticalmonk · 2017-04-24T09:44:06Z

@jreback Can this PR be reviewed please?

jreback · 2017-04-24T10:15:32Z

pandas/tests/dtypes/test_common.py

+            with tm.assertRaisesRegexp(TypeError, msg):
+                pandas_dtype(dtype)
+
+        valid_list = [object, 'float64', np.object_, list,


add np.dtype('object'); list is not a valid dtype.

'list is not a valid dtype'

@jreback That would result in code such as Series(dtype=list) raising an error.
Among other things, this test would break.

that's wrong it should raise
list is not a valid dtype

Should I modify this particular test and remove the checks involving list dtype?

yep

add list to the invalid types checking

jreback · 2017-04-24T10:16:26Z

pandas/tests/dtypes/test_common.py

@@ -13,6 +14,19 @@

 class TestPandasDtype(tm.TestCase):

+    # Passing invalid dtype, both as a string or object, must raise TypeError
+    def test_invalid_dtype_error(self):
+        msg = 'not understood'


add the issue number as a comment here

jreback · 2017-04-24T10:17:01Z

pandas/core/generic.py

@@ -164,13 +164,15 @@ def _validate_dtype(self, dtype):
        """ validate the passed dtype """

        if dtype is not None:
-            dtype = _coerce_to_dtype(dtype)
+            # This would raise an error if an invalid dtype was passed


comment not necessary

jreback · 2017-04-24T10:18:03Z

pandas/core/dtypes/common.py

+            np.dtype(dtype)
+        except (TypeError, ValueError):
+            raise
+        if dtype in [object, np.object_, list]:


list is not valid

jreback · 2017-04-24T10:18:43Z

pandas/core/dtypes/common.py

@@ -731,11 +731,23 @@ def pandas_dtype(dtype):
            except TypeError:
                pass

+        elif dtype == 'object':


catch this lower (where you catch object, np.dtype('object')

jreback · 2017-04-24T10:19:56Z

pandas/core/dtypes/common.py

+            raise
+        if dtype in [object, np.object_, list]:
+            return np.dtype(dtype)
+        elif np.dtype(dtype).kind == 'O':


doesn't np.dtype(dtype).kind == 'O' catch object, np.object_, 'object'?

@jreback From what I have understood, any invalid dtype (such as pd.Timestamp) should raise an error. np.dtype(invalid_type).kind = 0 for such objects. However, this will also catch some valid dtypes such as object, np.object_ and 'object' which we safeguard against by catching them earlier and returning np.dtype(valid_dtype) before this condition is evaluated.

ok add a comment then as this is a tricky case

jreback · 2017-04-24T11:46:35Z

pandas/core/dtypes/common.py

@@ -737,5 +737,20 @@ def pandas_dtype(dtype):
            pass
    elif isinstance(dtype, ExtensionDtype):
        return dtype
+    else:


we don't need this in the else, it IS the else.
then do

try: npdtype = np.dtype(dtype) except ... .... return npdtype

so you can replace usages of np.dtype(dtype) with npdtype

jreback · 2017-04-24T11:50:06Z

pandas/tests/test_strings.py

@@ -1210,7 +1210,6 @@ def test_empty_str_methods(self):
        empty_str = empty = Series(dtype=str)
        empty_int = Series(dtype=int)
        empty_bool = Series(dtype=bool)
-        empty_list = Series(dtype=list)


actually, this is the same as Series(dtype=str), so just change that one to Series(dtype=object) and done

jreback

small comments, ping when pushed and green.

jreback · 2017-04-24T11:51:24Z

this is ok for 0.20.0, pls add a whatsnew note (bug fix section is fine).

Changes made to pandas_dtype() in pandas/core/dtypes/common.py

Changes made to series.py, generic.py in pandas/core and cast.py in pandas/core/dtypes

This change ensures that no error is raised when type 'object' is passed.

analyticalmonk · 2017-04-24T15:18:29Z

@jreback Added the whatsnew note.

jreback · 2017-04-25T01:16:32Z

doc/source/whatsnew/v0.20.0.txt

@@ -1510,6 +1510,12 @@ Conversion
 - Bug in the return type of ``pd.unique`` on a ``Categorical``, which was returning an ndarray and not a ``Categorical`` (:issue:`15903`)
 - Bug in ``Index.to_series()`` where the index was not copied (and so mutating later would change the original), (:issue:`15949`)

+


just put it in conversion, no need to make a different section.

jreback · 2017-04-25T01:17:32Z

pandas/tests/test_strings.py

        tm.assert_series_equal(empty_int, empty.str.find('a'))
        tm.assert_series_equal(empty_int, empty.str.rfind('a'))
        tm.assert_series_equal(empty_str, empty.str.pad(42))
        tm.assert_series_equal(empty_str, empty.str.center(42))
-        tm.assert_series_equal(empty_list, empty.str.split('a'))


can you add back tests with empty_str so that all of the original functions are convered. don't duplicate tests though.

Added back the tests after looking out for duplicates; all green now.

Changes made to: - test_empty_str_methods in test_strings.py - test_invalid_dtype_error in dtypes/test_common.py

jreback · 2017-04-25T11:40:15Z

add a test for the original issue as well

pd.Series([], name='time',dtype=pd.Timestamp)
which works (should raise)

analyticalmonk · 2017-04-25T13:58:40Z

@jreback Should I add the test in core/tests/series/test_dtypes.py?

jreback · 2017-04-25T14:01:23Z

put in tests/series/test_constructors I think (this is an additional test), leave what you have

analyticalmonk · 2017-04-26T06:22:28Z

@jreback Anything else which is missing?

jreback · 2017-04-26T13:33:17Z

thanks @analyticalmonk

nice fixes!

keem em coming!

closes pandas-dev#15520 Author: Akash Tandon <[email protected]> Author: root <[email protected]> Author: analyticalmonk <[email protected]> Author: Akash Tandon <[email protected]> Closes pandas-dev#16047 from analyticalmonk/patch_for_15520 and squashes the following commits: 3646eb6 [analyticalmonk] TST: check for invalid dtype for Series constructor per GH15520 73d980a [Akash Tandon] Merge branch 'master' into patch_for_15520 b3c2fbb [root] BUG: Added 'O' to pandas_dtype's valid list c3699fb [root] DOC: added whatsnew entry for PR#16047 addressing GH15520 fbed5a6 [Akash Tandon] TST: Added list to invalid dtype ad9f345 [Akash Tandon] CLN: refactored code related to issue GH15520 a358181 [Akash Tandon] BUG: Added numpy.dtype_ to valid pandas_dtype() type list 3eaa432 [Akash Tandon] TST: Added numpy.object_ dtype to valid pandas_dtype list f858726 [Akash Tandon] style fix d4971cd [Akash Tandon] BUG: pandas_dtype() to raise error for invalid dtype per GH15520 ee0030f [Akash Tandon] TST: added more test-cases for pandas_dtype() test 3700259 [Akash Tandon] CLN: Replace _coerce_to_dtype() with pandas_dtype() c10e1d4 [Akash Tandon] TST: maintain list containing dtypes in TestPandasDtype fecba12 [Akash Tandon] BUG: Raise when invalid dtype passed to pandas_dtype 99fb660 [Akash Tandon] TST: wrote test representing bug fix result for pandas-dev#15520

analyticalmonk changed the title ~~bug fix to raise for invalid dtypes #15520~~ bug fix to raise for invalid dtypes per issue #15520 Apr 18, 2017

jreback reviewed Apr 18, 2017

View reviewed changes

jreback added Dtype Conversions Unexpected or buggy dtype conversions Error Reporting Incorrect or improved errors from pandas labels Apr 18, 2017

analyticalmonk force-pushed the patch_for_15520 branch 3 times, most recently from cfcc4c2 to a77ab66 Compare April 21, 2017 08:02

analyticalmonk changed the title ~~bug fix to raise for invalid dtypes per issue #15520~~ BUG: raise for invalid dtypes per issue #15520 Apr 21, 2017

analyticalmonk force-pushed the patch_for_15520 branch from 1d5f4b7 to 1f553e2 Compare April 21, 2017 14:00

jreback reviewed Apr 24, 2017

View reviewed changes

analyticalmonk force-pushed the patch_for_15520 branch from 1f553e2 to 72a53ff Compare April 24, 2017 11:08

jreback reviewed Apr 24, 2017

View reviewed changes

jreback added this to the 0.20.0 milestone Apr 24, 2017

analyticalmonk force-pushed the patch_for_15520 branch from 3aeee36 to 9d46787 Compare April 24, 2017 12:28

analyticalmonk added 2 commits April 24, 2017 19:38

TST: wrote test representing bug fix result for pandas-dev#15520

99fb660

BUG: Raise when invalid dtype passed to pandas_dtype

fecba12

Changes made to pandas_dtype() in pandas/core/dtypes/common.py

analyticalmonk added 8 commits April 24, 2017 19:39

TST: maintain list containing dtypes in TestPandasDtype

c10e1d4

CLN: Replace _coerce_to_dtype() with pandas_dtype()

3700259

Changes made to series.py, generic.py in pandas/core and cast.py in pandas/core/dtypes

TST: added more test-cases for pandas_dtype() test

ee0030f

BUG: pandas_dtype() to raise error for invalid dtype per GH15520

d4971cd

This change ensures that no error is raised when type 'object' is passed.

style fix

f858726

TST: Added numpy.object_ dtype to valid pandas_dtype list

3eaa432

BUG: Added numpy.dtype_ to valid pandas_dtype() type list

a358181

CLN: refactored code related to issue GH15520

ad9f345

analyticalmonk force-pushed the patch_for_15520 branch from 94a1f87 to 84ee765 Compare April 24, 2017 14:16

jreback reviewed Apr 25, 2017

View reviewed changes

TST: Added list to invalid dtype

fbed5a6

Changes made to: - test_empty_str_methods in test_strings.py - test_invalid_dtype_error in dtypes/test_common.py

analyticalmonk force-pushed the patch_for_15520 branch from 84ee765 to 61e614e Compare April 25, 2017 05:19

analyticalmonk added 2 commits April 25, 2017 10:57

DOC: added whatsnew entry for PR#16047 addressing GH15520

c3699fb

BUG: Added 'O' to pandas_dtype's valid list

b3c2fbb

analyticalmonk force-pushed the patch_for_15520 branch 2 times, most recently from 1a35178 to b3c2fbb Compare April 25, 2017 07:27

Merge branch 'master' into patch_for_15520

73d980a

analyticalmonk force-pushed the patch_for_15520 branch from 8148868 to ae7ecb0 Compare April 25, 2017 14:45

TST: check for invalid dtype for Series constructor per GH15520

3646eb6

analyticalmonk force-pushed the patch_for_15520 branch from ae7ecb0 to 3646eb6 Compare April 25, 2017 14:48

jreback closed this in 61ca022 Apr 26, 2017

		@@ -165,12 +166,15 @@ def _validate_dtype(self, dtype):

		if dtype is not None:
		dtype = _coerce_to_dtype(dtype)

		@@ -13,6 +14,15 @@

		class TestPandasDtype(tm.TestCase):

		# Passing invalid dtype, both as a string or object, must raise TypeError

		@@ -1510,6 +1510,12 @@ Conversion
		- Bug in the return type of ``pd.unique`` on a ``Categorical``, which was returning an ndarray and not a ``Categorical`` (:issue:`15903`)
		- Bug in ``Index.to_series()`` where the index was not copied (and so mutating later would change the original), (:issue:`15949`)

BUG: raise for invalid dtypes per issue #15520 #16047

BUG: raise for invalid dtypes per issue #15520 #16047

Conversation

analyticalmonk commented Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

analyticalmonk Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analyticalmonk Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

jreback Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

analyticalmonk commented Apr 20, 2017

codecov bot commented Apr 21, 2017

Codecov Report

codecov bot commented Apr 21, 2017 • edited Loading

Codecov Report

analyticalmonk commented Apr 24, 2017

Choose a reason for hiding this comment

analyticalmonk Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analyticalmonk Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analyticalmonk Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jreback commented Apr 24, 2017

analyticalmonk commented Apr 24, 2017

Choose a reason for hiding this comment

jreback Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Apr 25, 2017

analyticalmonk commented Apr 25, 2017

jreback commented Apr 25, 2017

analyticalmonk commented Apr 26, 2017

jreback commented Apr 26, 2017

analyticalmonk commented Apr 18, 2017 •

edited

Loading

analyticalmonk Apr 18, 2017 •

edited

Loading

analyticalmonk Apr 18, 2017 •

edited

Loading

jreback Apr 20, 2017 •

edited

Loading

codecov bot commented Apr 21, 2017 •

edited

Loading

analyticalmonk Apr 24, 2017 •

edited

Loading

analyticalmonk Apr 24, 2017 •

edited

Loading

analyticalmonk Apr 24, 2017 •

edited

Loading

jreback Apr 25, 2017 •

edited

Loading