Skip to content
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

Suppress incorrect warning in nargsort for timezone-aware DatetimeIndex #25629

Merged
merged 1 commit into from
Mar 11, 2019
Merged

Suppress incorrect warning in nargsort for timezone-aware DatetimeIndex #25629

merged 1 commit into from
Mar 11, 2019

Conversation

jorisvandenbossche
Copy link
Member

Closes #25439, alternative to #25595 for 0.24.2

@jorisvandenbossche jorisvandenbossche added this to the 0.24.2 milestone Mar 10, 2019
@jorisvandenbossche jorisvandenbossche mentioned this pull request Mar 10, 2019
Closed
jreback
jreback requested changes Mar 10, 2019
View reviewed changes
pandas/core/sorting.py
with warnings.catch_warnings():
# https://github.com/pandas-dev/pandas/issues/25439
# can be removed once ExtensionArrays are properly handled by nargsort
warnings.filterwarnings(
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we call .to_numpy() here instead? (I think think always is an index), or is this a performance issue? (until we have real EA support)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not opposed to this as an interim solution, just want to understand

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I understand, to_numpy has the future behaviour (i.e. converting to object dtype), while here we want to have the current M8[ns]

Copy link
Member Author

Choose a reason for hiding this comment

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

But of course, with to_numpy(dtype='datetime64[ns]'), you can get that.

In that case, we have to do a check here for datetimetz data; something like:

if is_datetimetz_dtype(items):
    items = items.to_numpy(dtype='datetime64[ns]')
else:
    items = np.asanyarray(items)

It's also custom code for this case, but if the above is preferable, happy to change (personally don't have a strong favor for either of them).

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah i think this needs to be sorted, but ok for now with your existing.

Copy link
Contributor

Choose a reason for hiding this comment

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

however, shall we leave this or a follow-on open to fix this/

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to expand PR #25595 to include a proper fix (for 0.25.0), involving a clean-up of EA.argsort, which will be the follow-up on this.
And apparently, I already opened an issue about the argsort API problem a few months ago: #21801 (will update that issue with the new information)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok cool.

@jreback jreback added the Compat pandas objects compatability with Numpy or Python functions label Mar 10, 2019
@codecov
Copy link

codecov bot commented Mar 10, 2019

Codecov Report

Merging #25629 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25629      +/-   ##
==========================================
+ Coverage   91.26%   91.26%   +<.01%     
==========================================
  Files         173      173              
  Lines       52968    52971       +3     
==========================================
+ Hits        48339    48342       +3     
  Misses       4629     4629
Flag Coverage Δ
#multiple 89.83% <100%> (ø) ⬆️
#single 41.71% <100%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/sorting.py 98.29% <100%> (+0.02%) ⬆️

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 e28ae70...cd11341. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Mar 10, 2019 •
edited
Loading

Codecov Report

Merging #25629 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25629      +/-   ##
==========================================
+ Coverage   91.26%   91.26%   +<.01%     
==========================================
  Files         173      173              
  Lines       52968    52971       +3     
==========================================
+ Hits        48339    48342       +3     
  Misses       4629     4629
Flag Coverage Δ
#multiple 89.83% <100%> (ø) ⬆️
#single 41.71% <100%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/sorting.py 98.29% <100%> (+0.02%) ⬆️

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 e28ae70...cd11341. Read the comment docs.

@jreback jreback merged commit de52d0b into pandas-dev:master Mar 11, 2019
@lumberbot-app
Copy link

lumberbot-app bot commented Mar 11, 2019

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout 0.24.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 de52d0b10c28318ae48390dbe7abcd1807a1154a
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #25629: Suppress incorrect warning in nargsort for timezone-aware DatetimeIndex'
  1. Push to a named branch :
git push YOURFORK 0.24.x:auto-backport-of-pr-25629-on-0.24.x
  1. Create a PR against branch 0.24.x, I would have named this PR:

"Backport PR #25629 on branch 0.24.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@TomAugspurger
Copy link
Contributor

I'll put up a backport PR for this.

@jorisvandenbossche jorisvandenbossche deleted the nargsort-filter-warning branch March 11, 2019 13:05
TomAugspurger added a commit to TomAugspurger/pandas that referenced this pull request Mar 11, 2019
@TomAugspurger
Backport PR pandas-dev#25629: Suppress incorrect warning in nargsort …
dbd4f0b 
…for timezone-aware DatetimeIndex
@TomAugspurger TomAugspurger mentioned this pull request Mar 11, 2019
Merged
jorisvandenbossche pushed a commit that referenced this pull request Mar 11, 2019
@TomAugspurger @jorisvandenbossche
Backport PR #25629: Suppress incorrect warning in nargsort for timezo…
f70f2b6 
…ne-aware DatetimeIndex (#25657)
thoo added a commit to thoo/pandas that referenced this pull request Mar 11, 2019
@thoo
Merge remote-tracking branch 'upstream/master' into pivot
aaa21bb 
* upstream/master: (110 commits)
  DOC: hardcode contributors for 0.24.x releases (pandas-dev#25662)
  DOC: restore toctree maxdepth (pandas-dev#25134)
  BUG: Redefine IndexOpsMixin.size, fix pandas-dev#25580. (pandas-dev#25584)
  BUG: to_csv line endings with compression (pandas-dev#25625)
  DOC: file obj for to_csv must be newline='' (pandas-dev#25624)
  Suppress incorrect warning in nargsort for timezone-aware DatetimeIndex (pandas-dev#25629)
  TST: fix incorrect sparse test (now failing on scipy master) (pandas-dev#25653)
  CLN: Removed debugging code (pandas-dev#25647)
  DOC: require Return section only if return is not None nor commentary (pandas-dev#25008)
  DOC:Remove hard-coded examples from _flex_doc_SERIES (pandas-dev#24589) (pandas-dev#25524)
  TST: xref pandas-dev#25630 (pandas-dev#25643)
  BUG: Fix pandas-dev#25481 by fixing the error message in TypeError (pandas-dev#25540)
  Fixturize tests/frame/test_mutate_columns.py (pandas-dev#25642)
  Fixturize tests/frame/test_join.py (pandas-dev#25639)
  Fixturize tests/frame/test_combine_concat.py (pandas-dev#25634)
  Fixturize tests/frame/test_asof.py (pandas-dev#25628)
  BUG: Fix user-facing AssertionError with to_html (pandas-dev#25608) (pandas-dev#25620)
  DOC: resolve all GL03 docstring validation errors (pandas-dev#25525)
  TST: failing wheel building on PY2 and old numpy (pandas-dev#25631)
  DOC: Remove makePanel from docs (pandas-dev#25609) (pandas-dev#25612)
  ...
@makbigc makbigc mentioned this pull request Jun 9, 2019
Closed
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

Assignees
No one assigned
Labels
Compat pandas objects compatability with Numpy or Python functions
Projects
None yet
Milestone
0.24.2
Development

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