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

CLN/DEPR: remove pd.ordered_merge #18459

Merged
merged 2 commits into from Nov 24, 2017

Conversation

Projects
None yet
3 participants
@topper-123
Contributor

topper-123 commented Nov 23, 2017

  • [ x] xref #13358
  • [x ] passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • [ x] whatsnew entry

pd.ordered_merge was deprecated in #13358 (pandas v.0.19). This PR removes it from the code base.

@codecov

This comment has been minimized.

codecov bot commented Nov 24, 2017

Codecov Report

Merging #18459 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18459      +/-   ##
==========================================
- Coverage   91.35%   91.34%   -0.02%     
==========================================
  Files         163      163              
  Lines       49695    49691       -4     
==========================================
- Hits        45401    45388      -13     
- Misses       4294     4303       +9
Flag Coverage Δ
#multiple 89.14% <100%> (-0.01%) ⬇️
#single 39.66% <100%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/reshape/api.py 100% <100%> (ø) ⬆️
pandas/core/reshape/merge.py 94.28% <100%> (-0.04%) ⬇️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
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 5e67065...e1b862a. Read the comment docs.

@codecov

This comment has been minimized.

codecov bot commented Nov 24, 2017

Codecov Report

Merging #18459 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18459      +/-   ##
==========================================
- Coverage   91.35%   91.34%   -0.02%     
==========================================
  Files         163      163              
  Lines       49695    49691       -4     
==========================================
- Hits        45401    45388      -13     
- Misses       4294     4303       +9
Flag Coverage Δ
#multiple 89.14% <100%> (-0.01%) ⬇️
#single 39.66% <100%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/reshape/api.py 100% <100%> (ø) ⬆️
pandas/core/reshape/merge.py 94.28% <100%> (-0.04%) ⬇️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
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 5e67065...e1b862a. Read the comment docs.

@codecov

This comment has been minimized.

codecov bot commented Nov 24, 2017

Codecov Report

Merging #18459 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18459      +/-   ##
==========================================
- Coverage   91.36%   91.34%   -0.02%     
==========================================
  Files         163      163              
  Lines       49704    49700       -4     
==========================================
- Hits        45411    45398      -13     
- Misses       4293     4302       +9
Flag Coverage Δ
#multiple 89.14% <100%> (-0.01%) ⬇️
#single 39.66% <100%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/reshape/merge.py 94.28% <ø> (-0.04%) ⬇️
pandas/core/reshape/api.py 100% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
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 aec3347...0e3f539. Read the comment docs.

@jsexauer jsexauer referenced this pull request Nov 24, 2017

Open

DEPR: deprecations from prior versions #6581

0 of 85 tasks complete

@jorisvandenbossche jorisvandenbossche changed the title from DEPR: remove pd.ordered_merge to CLN/DEPR: remove pd.ordered_merge Nov 24, 2017

@jorisvandenbossche jorisvandenbossche added this to the 0.22.0 milestone Nov 24, 2017

@jorisvandenbossche

Some comments, but looks good for the rest!

try:
from pandas import merge_ordered
except ImportError:
from pandas import ordered_merge as merge_ordered

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Nov 24, 2017

Member

You can still leave this one here (in case you want to compare a benchmark with pandas 0.18, which is quite unlikely, so we need to decide at a certain point how to deal with such things in the asv benchmarks, but let's leave that for another issue)

This comment has been minimized.

@topper-123

topper-123 Nov 24, 2017

Contributor

I'm not sure I understand: merge_ordered will always exist, so the ImportError is never reached and hence the try/except can be removed. Or am I misunderstanding something?

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Nov 24, 2017

Member

Eg in pandas 0.18.0 merge_ordered does not exist, and then the except part will be reached. The thing with benchmarks is that you run the latest (master) version of the benchmark also on older code (so benchmarks in master do not only need to satisfy master itself)

This comment has been minimized.

@topper-123

topper-123 Nov 24, 2017

Contributor

Ok, I understand, thansk for explaining. I will update the PR later tonight.

@@ -1083,7 +1067,7 @@ def _get_join_indexers(left_keys, right_keys, sort=False, how='inner',
class _OrderedMerge(_MergeOperation):
_merge_type = 'ordered_merge'
_merge_type = 'merge_ordered'

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Nov 24, 2017

Member

This one should not be changed I think. _merge_type is passed to __finalize__, so is mainly meant for subclasses being able to do something special. So we can leave that intact (only very advanced use case anyhow, it is not visible to normal user)

This comment has been minimized.

@topper-123

topper-123 Nov 24, 2017

Contributor

I don't understand this either: ordered_merge is removed, so calling it in that finanlize would cause an AttributeError. What am I missing here?

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Nov 24, 2017

Member

ordered_merge and merge_ordered where just aliases for the same underlying code, so both have been using _merge_type = ordered_merge. Changing this could break subclasses (as you are changing it for merge_ordered)

@jreback jreback merged commit 6660638 into pandas-dev:master Nov 24, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Contributor

jreback commented Nov 24, 2017

thanks @topper-123

@jreback jreback referenced this pull request Nov 24, 2017

Open

DEPR: deprecations log for removed issues #13777

115 of 115 tasks complete

@topper-123 topper-123 deleted the topper-123:remove_ordered_merge branch Nov 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment