-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
[#19431] Regression in make_block_same_class (tests failing for new fastparquet release) #19434
Changes from 11 commits
eb55928
2f4fc07
e18f8e6
9a07d97
3d9810d
ee75fdf
68d6324
985081d
0cfcd37
0c4a6d7
6ce68cf
f414743
0d76fe7
bb95dc6
d9a2e2a
97b17e9
800b741
52220b9
c602a76
ddbbde3
6e6b5f0
326394f
77422ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can add this back, but it needs to be deprecated anyhow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fp should be using |
||
""" | ||
Create a new block, with type inference propagate any values that are | ||
not specified | ||
|
@@ -216,20 +216,21 @@ 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe remove dtype here? As fastparquet was not using it (and I don't think anybody else will) |
||
|
||
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. deprecate dtype here (add as a FutureWarning) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
""" Wrap given values in a block of same type as self. """ | ||
if placement is None: | ||
placement = self.mgr_locs | ||
return make_block(values, placement=placement, ndim=ndim, | ||
klass=self.__class__) | ||
klass=self.__class__, dtype=dtype) | ||
|
||
def __unicode__(self): | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,6 +71,15 @@ def fp(): | |
return 'fastparquet' | ||
|
||
|
||
@pytest.fixture | ||
def fp_lt_014(): | ||
if not _HAVE_FASTPARQUET: | ||
pytest.skip("fastparquet is not installed") | ||
if LooseVersion(fastparquet.__version__) >= LooseVersion('0.1.4'): | ||
pytest.skip("fastparquet is >= 0.1.4") | ||
return 'fastparquet' | ||
|
||
|
||
@pytest.fixture | ||
def df_compat(): | ||
return pd.DataFrame({'A': [1, 2, 3], 'B': 'foo'}) | ||
|
@@ -448,9 +457,11 @@ class TestParquetFastParquet(Base): | |
def test_basic(self, fp, df_full): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make 2 tests here, one for < 0.1.4 and 1 for > |
||
df = df_full | ||
|
||
# additional supported types for fastparquet | ||
# additional supported types for fastparquet>=0.1.4 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can leave out the ">=0.1.4" as the timedelta is not specific for the newer versions |
||
if LooseVersion(fastparquet.__version__) >= LooseVersion('0.1.4'): | ||
df['datetime_tz'] = pd.date_range('20130101', periods=3, | ||
tz='US/Eastern') | ||
df['timedelta'] = pd.timedelta_range('1 day', periods=3) | ||
|
||
check_round_trip(df, fp) | ||
|
||
@pytest.mark.skip(reason="not supported") | ||
|
@@ -482,14 +493,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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same need 2 tests (1 with each fixture) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pyarrow is using similar style. there are already two tests for datetime_tz, one < 0.1.4 one >. |
||
def test_datetime_tz(self, fp_lt_014): | ||
|
||
# fastparquet<0.1.4 doesn't preserve tz | ||
df = pd.DataFrame({'a': pd.date_range('20130101', periods=3, | ||
tz='US/Eastern')}) | ||
|
||
# warns on the coercion | ||
with catch_warnings(record=True): | ||
check_round_trip(df, fp, expected=df.astype('datetime64[ns]')) | ||
check_round_trip(df, fp_lt_014, | ||
expected=df.astype('datetime64[ns]')) | ||
|
||
def test_filter_row_groups(self, fp): | ||
d = {'a': list(range(0, 3))} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean
read_parquet
instead ofdate_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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, I am not sure how this whatsnew notice proposed relates to API catalogue. Do you want me to remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
off to bed now. thanks guys. hope CI will work normally after this PR.