Skip to content

ERR: Improve error message on non-sorted input with .truncate #17984

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 6 commits into from
Nov 17, 2017
Merged

ERR: Improve error message on non-sorted input with .truncate #17984

merged 6 commits into from
Nov 17, 2017

Conversation

reidy-p
Copy link
Contributor

@reidy-p reidy-p commented Oct 25, 2017 •
edited
Loading

@codecov
Copy link

codecov bot commented Oct 25, 2017 •
edited
Loading

Codecov Report

Merging #17984 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17984      +/-   ##
==========================================
- Coverage   91.39%   91.36%   -0.04%     
==========================================
  Files         164      164              
  Lines       49854    49782      -72     
==========================================
- Hits        45566    45484      -82     
- Misses       4288     4298      +10
Flag Coverage Δ
#multiple 89.16% <100%> (-0.02%) ⬇️
#single 39.49% <50%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/generic.py 95.73% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/tseries/offsets.py 96.91% <0%> (-0.16%) ⬇️
pandas/core/panel.py 97.14% <0%> (-0.15%) ⬇️
pandas/core/frame.py 97.8% <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 c2590b3...f1a7616. Read the comment docs.

@gfyoung gfyoung added Error Reporting Incorrect or improved errors from pandas Datetime Datetime data dtype labels Oct 25, 2017
@TomAugspurger TomAugspurger added this to the 0.21.1 milestone Oct 26, 2017
@jreback jreback removed this from the 0.21.1 milestone Oct 27, 2017
jreback
jreback requested changes Oct 27, 2017
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

can you add a note in other api changes in 0.22 (this is technially an api change, sort of a trivial one)

pandas/tests/frame/test_timeseries.py
@@ -374,6 +374,31 @@ def test_truncate_copy(self):
truncated.values[:] = 5.
assert not (self.tsframe.values[5:11] == 5).any()

def test_truncate_nonsortedindex(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a test for series as well (you can also put this test generically in test_generic if its easier)

jreback
jreback reviewed Oct 27, 2017
View reviewed changes
pandas/core/generic.py
@@ -6397,6 +6397,10 @@ def truncate(self, before=None, after=None, axis=None, copy=True):
axis = self._get_axis_number(axis)
ax = self._get_axis(axis)

Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment here

@jreback
Copy link
Contributor

jreback commented Nov 12, 2017

can you rebase

jreback
jreback requested changes Nov 16, 2017
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

looks good, except you have lint issues, you can run make lint-diff and see

pandas/core/generic.py Outdated
# GH 17935
# Check that index is sorted
if (not ax.is_monotonic_increasing and
not ax.is_monotonic_decreasing):
Copy link
Contributor

Choose a reason for hiding this comment

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

lint error

jreback
jreback reviewed Nov 16, 2017
View reviewed changes
doc/source/whatsnew/v0.22.0.txt Outdated
@@ -46,6 +46,8 @@ Other API Changes
- :class:`Timestamp` will no longer silently ignore invalid ``freq`` arguments (:issue:`5168`)
- :class:`CacheableOffset` and :class:`WeekDay` are no longer available in the ``pandas.tseries.offsets`` module (:issue:`17830`)
- `tseries.frequencies.get_freq_group()` and `tseries.frequencies.DAYS` are removed from the public API (:issue:`18034`)
- :func:`Series.truncate` and :func:`DataFrame.truncate` will raise a ``ValueError`` if the index is not sorted (:issue:`17935`)
Copy link
Contributor

Choose a reason for hiding this comment

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

say, rather than a non-helpful KeyError

@reidy-p
Copy link
Contributor Author

reidy-p commented Nov 16, 2017 •
edited
Loading

I updated the whatsnew and think I fixed the lint issue.

But since I opened this PR a new test_truncate test has been added to pandas/tests/series/test_period.py in #17755. This new test fails on my branch because it tries to truncate on a non-sorted index. So I modified the test to sort the index before truncating and now it passes. Alternatively, I could simply check for the new error message ValueError: truncate requires a sorted index in this test.

@jreback jreback added this to the 0.22.0 milestone Nov 17, 2017
@jreback jreback merged commit c1406a3 into pandas-dev:master Nov 17, 2017
@jreback
Copy link
Contributor

jreback commented Nov 17, 2017

thanks @reidy-p

changing the test for Period was just fine

@reidy-p reidy-p deleted the truncate_errormsg branch November 27, 2017 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

@TomAugspurger TomAugspurger TomAugspurger approved these changes

@gfyoung gfyoung gfyoung approved these changes

Assignees
No one assigned
Labels
Datetime Datetime data dtype Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.

ERR: better error message on non-sorted input with .truncate
4 participants
@reidy-p @jreback @TomAugspurger @gfyoung