[#19431] Regression in make_block_same_class (tests failing for new fastparquet release) #19434

minggli · 2018-01-28T18:59:58Z

closes Regression in make_block_same_class (tests failing for new fastparquet release) #19431
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

dtype seems still in use at:
https://github.com/minggli/pandas/blob/2f4fc0790a5e5c51eb80bbfadf4c80f0bb424c56/pandas/core/internals.py#L2911

jorisvandenbossche

Thanks for working on this!

Can you add a test for the internal itself? You can take the small example I gave here (#19431 (comment)) and put it in somewhere in tests/internals/test_internals.py (probably best in the TestBlock class)

jorisvandenbossche · 2018-01-28T19:44:09Z

pandas/tests/io/test_parquet.py

        # warns on the coercion
        with catch_warnings(record=True):
-            check_round_trip(df, fp, expected=df.astype('datetime64[ns]'))
+            check_round_trip(df, fp)


You will need to do this dependent on the fastparquet version, as older versions still return non-tz data (or we could skip the test for older versions)

or probably better: check how it is done for Arrow (test_basic in TestParquetPyarrow), it adds a tz column to the tested dataframe dependent on the version.

codecov · 2018-01-28T20:28:03Z

Codecov Report

❗ No coverage uploaded for pull request base (master@8cbee35). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #19434   +/-   ##
=========================================
  Coverage          ?   91.62%           
=========================================
  Files             ?      150           
  Lines             ?    48724           
  Branches          ?        0           
=========================================
  Hits              ?    44642           
  Misses            ?     4082           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`89.99% <100%> (?)`
#single	`41.74% <66.66%> (?)`

Impacted Files	Coverage Δ
pandas/core/internals.py	`95.47% <100%> (ø)`

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 8cbee35...77422ba. Read the comment docs.

jreback · 2018-01-28T20:45:46Z

pandas/core/internals.py

@@ -206,7 +206,7 @@ def array_dtype(self):
        """
        return self.dtype

-    def make_block(self, values, placement=None, ndim=None):
+    def make_block(self, values, placement=None, ndim=None, dtype=None):


you can add this back, but it needs to be deprecated anyhow.

Then please answer in the original issue how to achieve the same result (see the code example I posted there, to create a new DatetimeTZ block from a numpy array).
But I don't see why this needs to be deprecated. The dtype argument was actually doing something, in contrast to fastpath (which we therefore deprecated)

fp should be using make_block and not make_block_same_class

jreback · 2018-01-28T20:46:26Z

pandas/tests/io/test_parquet.py

+        df = pd.DataFrame({'dt': pd.date_range('20130101', periods=3)})
+
+        # fastparquet supports timezone since 0.1.4, and not before
+        import fastparquet


you need to do this in a separate test. Have 1 more fp <= 0.1.3 and 1 for fp > 0.1.3 (essentially duplicate the test with 1 working for the old version and 1 for the new)

need to add 1 build with fp pinned to 0.1.3, the so pin in requirements-2.7.sh

@minggli we can indeed keep a test for the old behaviour (so only run for for older versions), but can you move the new behaviour to test_basic of this class (so we use the same pattern as in the pyarrow test), that is what I meant with my previous issue.

need to add 1 build with fp pinned to 0.1.3, the so pin in requirements-2.7.sh

@jreback We have one build for fastparquet 0.1.1 (that's the reason the tests were failing here in the original PR), so I would say that is enough?

jreback · 2018-01-28T20:53:42Z

I am going to merge #19435 before this to fix the CI for now. you should remove the pins (all except the 2.7 one), and this should still pass (after fixes above).

jorisvandenbossche · 2018-01-28T21:23:01Z

pandas/tests/io/test_parquet.py

@@ -483,10 +488,12 @@ def test_categorical(self, fp):
        check_round_trip(df, fp)

    def test_datetime_tz(self, fp):
-        # doesn't preserve tz
+        if LooseVersion(fastparquet.__version__) > LooseVersion('0.1.3'):
+            pytest.skip("timezone not supported for older fp")


maybe rather "timezones supported in newer versions of fp"

jreback · 2018-01-28T21:36:16Z

pandas/tests/io/test_parquet.py

@@ -448,6 +448,11 @@ class TestParquetFastParquet(Base):
    def test_basic(self, fp, df_full):
        df = df_full

+        # additional supported types for fastparquet >= 0.1.4


pls follow the existing style. see how we have a fixture for pa_ge_070. you need to make similar for fp

this is the existing style (see how it is for pyarrow) and what I asked for, so please leave it like this (in this PR).

For the old test we can use a fixture like fp_lt_014 however.

good point!

little confused. But I agree it's better to match existing style in test_basic to avoid unnecessary style variation.

this is a totally incorrect style ATM. So please see what you can fix. skipping INSIDE a test function is not correct, rather a fixture should be used. This must have slipped in.

Jeff, please leave this. I asked @minggli to do this, and it is the current style. You can do a follow-up PR if you want to change this in all places.

jreback

fix the 2.7 build (in requirements.sh) to use fastparquet=0.1.3

jorisvandenbossche · 2018-01-28T22:10:00Z

pandas/tests/io/test_parquet.py

+
+
+@pytest.fixture
+def fp_ge_014():


I don't think you are already using this one?

correct. removed.

jreback · 2018-01-28T22:14:25Z

pandas/tests/io/test_parquet.py

@@ -448,9 +466,11 @@ class TestParquetFastParquet(Base):
    def test_basic(self, fp, df_full):


make 2 tests here, one for < 0.1.4 and 1 for >

jreback · 2018-01-28T22:14:43Z

pandas/tests/io/test_parquet.py

@@ -482,14 +502,15 @@ def test_categorical(self, fp):
        df = pd.DataFrame({'a': pd.Categorical(list('abc'))})
        check_round_trip(df, fp)

-    def test_datetime_tz(self, fp):
-        # doesn't preserve tz


same need 2 tests (1 with each fixture)

pyarrow is using similar style. there are already two tests for datetime_tz, one < 0.1.4 one >.

jreback · 2018-01-28T22:15:57Z

add a whatsnew entry in other api changes for this. (say compat with fp >= 0.1.4)

jreback · 2018-01-28T22:28:21Z

pandas/core/internals.py


    def make_block_scalar(self, values):
        """
        Create a ScalarBlock
        """
        return ScalarBlock(values)

-    def make_block_same_class(self, values, placement=None, ndim=None):
+    def make_block_same_class(self, values, placement=None, ndim=None,
+                              dtype=None):


deprecate dtype here (add as a FutureWarning)

jreback · 2018-01-28T22:29:04Z

pandas/tests/io/test_parquet.py

@@ -448,9 +466,11 @@ class TestParquetFastParquet(Base):
    def test_basic(self, fp, df_full):
        df = df_full

-        # additional supported types for fastparquet
+        # additional supported types for fastparquet>=0.1.4
+        if LooseVersion(pyarrow.__version__) >= LooseVersion('0.1.4'):


this is wrong, you mean fastparquet, yes? This is why the fixtures are FAR preferred

Hi Jeff, I am fine with either however, Joris suggested for now to keep it in line with pyarrow style as per #19434 (comment). I feel it's good to keep it consistent and follow up with another PR for styling for both PyArrow and FP.

@minggli only the 'pyarrow' -> 'fastparquet' on this line you need to change anyway (copy paste error)

fixed. 👍

I suppose its ok. The problem is, this will be inconsistent with ALL the rest of pandas. You can make a simple change here to fix it. I would appreciate this done here, or ok in a followup . We spend an enormous amount of time making things consistent. adding inconsistent code is not great, even for expediency.

jorisvandenbossche · 2018-01-28T22:52:36Z

doc/source/whatsnew/v0.23.0.txt

@@ -312,6 +312,7 @@ Other API Changes
 - :func:`DatetimeIndex.shift` and :func:`TimedeltaIndex.shift` will now raise ``NullFrequencyError`` (which subclasses ``ValueError``, which was raised in older versions) when the index object frequency is ``None`` (:issue:`19147`)
 - Addition and subtraction of ``NaN`` from a :class:`Series` with ``dtype='timedelta64[ns]'`` will raise a ``TypeError` instead of treating the ``NaN`` as ``NaT`` (:issue:`19274`)
 - Set operations (union, difference...) on :class:`IntervalIndex` with incompatible index types will now raise a ``TypeError`` rather than a ``ValueError`` (:issue:`19329`)
+- Compatibility in :func:`pandas.date_range` with fastparquet==0.1.4 which now supports timezone e.g. ``tz``='US/Eastern'`` (:issue:`19431`)


Did you mean read_parquet instead of date_range ?
But, we didn't do anything for this, fastparquet 0.1.4 already works nicely with pandas 0.22, so I don't think this needs a whatsnew notice.

correct, I am not sure how this whatsnew notice proposed relates to API catalogue. Do you want me to remove this?

well, we are fixing something that we changed in the unreleased version of pandas, so users should not be aware of this. So I would remove this, but let's wait what @jreback says

We should however fix the note in io.rst ( the note in http://pandas-docs.github.io/pandas-docs-travis/io.html#parquet) about fastparquet not supporting timezones.

off to bed now. thanks guys. hope CI will work normally after this PR.

minggli · 2018-01-29T10:07:18Z

@jorisvandenbossche @jreback please feedback on this PR?

jorisvandenbossche

Can you add a test for the make_block_same_class deprecation? (one that assures the warning is raised, and that it is still working correctly)

jorisvandenbossche · 2018-01-29T10:13:00Z

pandas/core/internals.py

+        if dtype is not None:
+            # issue 19431 fastparquet is passing this
+            warnings.warn("dtype argument is deprecated, will be removed "
+                          "in a future release.", FutureWarning)


Let's make this a DeprecationWarning instead of FutureWarning, as it is typically only something developers need to see, not users.

post an issue on fastparquet to fix this as well

let’s leave this as a FutureWarning
it will encourage fp to fix this as it will be visible

We can use the issue I opened yesterday: dask/fastparquet#297

FutureWarning will just be annoying for users in this case, and I am confident fastparquet will change that

if it’s changed then we can simply move the bar on fp min version and this is no problem

we are not keeping private API around for external packages
this is way too much work

gents, which is it? FutureWarning or DeprecationWarning? it seems to me that DeprecationWarning is meant to be used here.

pls leave it as FutureWarning

jorisvandenbossche · 2018-01-29T10:13:31Z

pandas/tests/io/test_parquet.py

@@ -448,9 +457,11 @@ class TestParquetFastParquet(Base):
    def test_basic(self, fp, df_full):
        df = df_full

-        # additional supported types for fastparquet
+        # additional supported types for fastparquet>=0.1.4


you can leave out the ">=0.1.4" as the timedelta is not specific for the newer versions

jorisvandenbossche · 2018-01-29T10:14:36Z

pandas/core/internals.py

@@ -216,20 +216,25 @@ def make_block(self, values, placement=None, ndim=None):
        if ndim is None:
            ndim = self.ndim

-        return make_block(values, placement=placement, ndim=ndim)
+        return make_block(values, placement=placement, ndim=ndim, dtype=dtype)


maybe remove dtype here? As fastparquet was not using it (and I don't think anybody else will)
Or otherwise add the same deprecation warning as you did for make_block_same_class

…_class

jreback · 2018-01-29T11:06:28Z

doc/source/io.rst

@@ -4537,7 +4537,7 @@ See the documentation for `pyarrow <http://arrow.apache.org/docs/python/>`__ and
 .. note::

   These engines are very similar and should read/write nearly identical parquet format files.
-   Currently ``pyarrow`` does not support timedelta data, and ``fastparquet`` does not support timezone aware datetimes (they are coerced to UTC).
+   Currently ``pyarrow`` does not support timedelta data, ``fastparquet`` now supports timezone aware datetimes.


this needs to change. you can't say 'now supports' there is no context here. Simply remove the fp statement (or qualify it with fp >1.4)

jreback · 2018-01-29T11:07:07Z

pandas/tests/internals/test_internals.py

+        with pytest.warns(FutureWarning):
+            copy = block.make_block_same_class(block.values,
+                                               dtype=block.values.dtype)
+        assert block.dtype == copy.dtype


you don't need these asserts, just running this is enough

jreback · 2018-01-29T11:07:16Z

pandas/tests/internals/test_internals.py

@@ -285,6 +285,14 @@ def test_delete(self):
        with pytest.raises(Exception):
            newb.delete(3)

+    def test_make_block_same_class(self):
+        block = create_block('M8[ns, US/Eastern]', [3])


add the issue number as a comment

jreback · 2018-01-29T11:25:12Z

pandas/tests/internals/test_internals.py

+    def test_make_block_same_class(self):
+        # issue 19431
+        block = create_block('M8[ns, US/Eastern]', [3])
+        with pytest.warns(FutureWarning):


use tm.assert_produces_warning instead. we catch this usage of pytest.warns in the linter and is not standard for pandas codease.

good point.

jorisvandenbossche · 2018-01-29T12:43:15Z

@minggli Thanks a lot! (and sorry for the extensive back and forth)

minggli · 2018-01-29T12:45:33Z

@jorisvandenbossche no problem. happy to help!

…t release) (pandas-dev#19434)

…andas-dev#19434

minggli added 2 commits January 28, 2018 18:56

udpate test_parquet since fastparquet now handles tz

eb55928

bring back dtype kwarg because it is needed for DatetimeTZBlock

2f4fc07

jorisvandenbossche reviewed Jan 28, 2018

View reviewed changes

version dependence test_datetime_tz

e18f8e6

jreback requested changes Jan 28, 2018

View reviewed changes

jreback added the IO Parquet parquet, feather label Jan 28, 2018

minggli added 2 commits January 28, 2018 21:16

separate test cases for new and old behaviour of fastparquet

9a07d97

tidy test_datetime_tz to test old behaviour of fastparquet<0.14

3d9810d

jorisvandenbossche reviewed Jan 28, 2018

View reviewed changes

rephase reason to skip test case for oldder fp

ee75fdf

jreback requested changes Jan 28, 2018

View reviewed changes

minggli added 2 commits January 28, 2018 21:54

follow pytest fixture pattern as in pyarrow

68d6324

follow pyarrow test_basic style for fastparquet new behaviour>=0.1.4

985081d

jreback requested changes Jan 28, 2018

View reviewed changes

jorisvandenbossche reviewed Jan 28, 2018

View reviewed changes

jreback requested changes Jan 28, 2018

View reviewed changes

fastparquet=0.1.3

0cfcd37

jreback requested changes Jan 28, 2018

View reviewed changes

minggli added 2 commits January 28, 2018 22:45

other api change

0c4a6d7

fix typo

6ce68cf

jorisvandenbossche reviewed Jan 28, 2018

View reviewed changes

minggli added 3 commits January 28, 2018 22:56

deprecation warning for dtype in make_block_same_class.

f414743

Future warning for dtype in make_block_same_class.

0d76fe7

update notes as fastparquet nows supports timezone

bb95dc6

jsexauer mentioned this pull request Jan 28, 2018

DEPR: Clean up list of deprecations from prior versions #6581

Closed

1 task

minggli added 2 commits January 29, 2018 00:16

remove fastparquet pin

d9a2e2a

remove other api change as it is internal

97b17e9

jorisvandenbossche reviewed Jan 29, 2018

View reviewed changes

minggli added 4 commits January 29, 2018 10:21

remove version

800b741

remove dtype for make_block and DeprecationWarning on make_block_same…

52220b9

…_class

FutureWarning on make_block_same_class

c602a76

test case for dtype and warning generation

ddbbde3

jreback requested changes Jan 29, 2018

View reviewed changes

minggli added 2 commits January 29, 2018 11:14

issue number and simplify test

6e6b5f0

misc doc

326394f

jreback reviewed Jan 29, 2018

View reviewed changes

use pandas warning assert

77422ba

jorisvandenbossche merged commit 272cabb into pandas-dev:master Jan 29, 2018

minggli deleted the bugfixs/19431 branch January 29, 2018 12:46

jorisvandenbossche mentioned this pull request Jan 29, 2018

Change Future to DeprecationWarning for make_block_same_class #19442

Merged

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018

Regression in make_block_same_class (tests failing for new fastparque…

e58292f

…t release) (pandas-dev#19434)

jbrockmendel added a commit to jbrockmendel/pandas that referenced this pull request Jul 1, 2019

DEPR: remove pyarrow/fastparquet deprecations from pandas-dev#19265, p…

afa7611

…andas-dev#19434

jbrockmendel mentioned this pull request Jul 1, 2019

CLN: remove fastparquet compat shim #27170

Closed

jreback mentioned this pull request Nov 22, 2019

DEPR: deprecations log for removed issues #13777

Closed

		@@ -448,9 +466,11 @@ class TestParquetFastParquet(Base):
		def test_basic(self, fp, df_full):

[#19431] Regression in make_block_same_class (tests failing for new fastparquet release) #19434

[#19431] Regression in make_block_same_class (tests failing for new fastparquet release) #19434

Conversation

minggli commented Jan 28, 2018 • edited Loading

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 28, 2018 • edited Loading

Codecov Report

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

jreback commented Jan 28, 2018

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

jreback left a 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

minggli Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

jreback commented Jan 28, 2018

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

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

minggli commented Jan 29, 2018

jorisvandenbossche left a 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

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

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

Choose a reason for hiding this comment

jorisvandenbossche commented Jan 29, 2018

minggli commented Jan 29, 2018

minggli commented Jan 28, 2018 •

edited

Loading

codecov bot commented Jan 28, 2018 •

edited

Loading

minggli Jan 28, 2018 •

edited

Loading