CLN/DEPR: remove pd.ordered_merge by topper-123 · Pull Request #18459 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation12 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- [ x] xref ENH: add pd.asof_merge #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.
DEPR: remove pd.ordered_merge CLN/DEPR: remove pd.ordered_merge
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, but looks good for the rest!
| try: |
|---|
| from pandas import merge_ordered |
| except ImportError: |
| from pandas import ordered_merge as merge_ordered |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I understand, thansk for explaining. I will update the PR later tonight.
| class _OrderedMerge(_MergeOperation): |
| _merge_type = 'ordered_merge' |
| _merge_type = 'merge_ordered' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
tp added 2 commits
