Skip to content

Fixed construction / factorization of empty PA and IA #24599

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 3, 2019
Merged

Fixed construction / factorization of empty PA and IA #24599

merged 2 commits into from
Jan 3, 2019

Conversation

TomAugspurger
Copy link
Contributor

Closes #23933

@TomAugspurger TomAugspurger added Period Period data type Interval Interval data type labels Jan 3, 2019
@TomAugspurger TomAugspurger added this to the 0.24.0 milestone Jan 3, 2019
jreback
jreback reviewed Jan 3, 2019
View reviewed changes
pandas/tests/extension/json/array.py
@@ -179,6 +179,9 @@ def _concat_same_type(cls, to_concat):

def _values_for_factorize(self):
frozen = self._values_for_argsort()
Copy link
Contributor

Choose a reason for hiding this comment

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

really?, why is it 2d to begin with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think because np.array([()]) is 2-D.

@codecov
Copy link

codecov bot commented Jan 3, 2019

Codecov Report

Merging #24599 into master will decrease coverage by 49.34%.
The diff coverage is 57.14%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #24599       +/-   ##
===========================================
- Coverage   92.38%   43.04%   -49.35%     
===========================================
  Files         166      166               
  Lines       52478    52485        +7     
===========================================
- Hits        48483    22593    -25890     
- Misses       3995    29892    +25897
Flag Coverage Δ
#multiple ?
#single 43.04% <57.14%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/interval.py 38.04% <0%> (-55.01%) ⬇️
pandas/core/arrays/period.py 43.03% <80%> (-55.49%) ⬇️
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/core/categorical.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/tseries/converter.py 0% <0%> (-100%) ⬇️
pandas/io/formats/html.py 0% <0%> (-99.37%) ⬇️
pandas/core/groupby/categorical.py 0% <0%> (-95.46%) ⬇️
pandas/io/sas/sas7bdat.py 0% <0%> (-91.17%) ⬇️
... and 126 more

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 62506ca...572f1d0. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 3, 2019 •
edited
Loading

Codecov Report

Merging #24599 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24599      +/-   ##
==========================================
- Coverage   92.38%   92.38%   -0.01%     
==========================================
  Files         166      166              
  Lines       52478    52485       +7     
==========================================
+ Hits        48483    48489       +6     
- Misses       3995     3996       +1
Flag Coverage Δ
#multiple 90.81% <100%> (ø) ⬆️
#single 43.05% <57.14%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/period.py 98.53% <100%> (+0.01%) ⬆️
pandas/core/arrays/interval.py 93.08% <100%> (+0.04%) ⬆️
pandas/util/testing.py 88% <0%> (-0.1%) ⬇️

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 62506ca...572f1d0. Read the comment docs.

TomAugspurger
TomAugspurger commented Jan 3, 2019
View reviewed changes
pandas/tests/arrays/test_period.py
@@ -225,8 +225,7 @@ def test_sub_period():
def test_where_different_freq_raises(other):
ser = pd.Series(period_array(['2000', '2001', '2002'], freq='D'))
cond = np.array([True, False, True])
with pytest.raises(IncompatibleFrequency,
match="Input has different freq=H"):
with pytest.raises(IncompatibleFrequency, match="freq"):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed this change, as __setitem__ might raise when creating the PeriodArray from a PeriodArray with different freq now, with a slightly different error message.

We might want to change validate_dtype_freq to use the error message from raise_on_incompatible. I haven't looked closely.

@jreback jreback merged commit c9a0405 into pandas-dev:master Jan 3, 2019
@jreback
Copy link
Contributor

jreback commented Jan 3, 2019

thanks!

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
@TomAugspurger @Pingviinituutti
Fixed construction / factorization of empty PA and IA (pandas-dev#24599)
0e93059
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
@TomAugspurger @Pingviinituutti
Fixed construction / factorization of empty PA and IA (pandas-dev#24599)
471d0d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback left review comments

Assignees
No one assigned
Labels
Interval Interval data type Period Period data type
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.
2 participants
@TomAugspurger @jreback