Skip to content

Conversation

Giftlin
Copy link
Contributor

@Giftlin Giftlin commented Oct 15, 2017

@@ -2183,7 +2183,7 @@ def _get_consensus_name(self, other):
return self._shallow_copy(name=name)
return self

def union(self, other):
def union(self, other, sort=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to implement this in subclasses of Index, eg. DatetimeIndex.

[pandas] find ./pandas -type f | grep .py$ |  xargs grep -n "def union"                                 17:41:36  ☁  fix-drop_duplicates-when-duplicate-column-names ☂ ✭
./pandas/core/common.py:353:def union(*seqs):
./pandas/core/dtypes/concat.py:214:def union_categoricals(to_union, sort_categories=False, ignore_order=False):
./pandas/core/indexes/base.py:2186:    def union(self, other):
./pandas/core/indexes/datetimes.py:983:    def union(self, other):
./pandas/core/indexes/datetimes.py:1036:    def union_many(self, others):
./pandas/core/indexes/multi.py:2584:    def union(self, other):
./pandas/core/indexes/range.py:416:    def union(self, other):
./pandas/core/indexes/timedeltas.py:496:    def union(self, other):

Copy link
Member

Choose a reason for hiding this comment

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

@Giftlin : I don't think you have addressed this comment yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah.. will do it 👍

@codecov
Copy link

codecov bot commented Oct 15, 2017

Codecov Report

Merging #17878 into master will increase coverage by <.01%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17878      +/-   ##
==========================================
+ Coverage   91.23%   91.24%   +<.01%     
==========================================
  Files         163      163              
  Lines       50102    50106       +4     
==========================================
+ Hits        45712    45719       +7     
+ Misses       4390     4387       -3
Flag Coverage Δ
#multiple 89.05% <84.61%> (+0.02%) ⬆️
#single 40.31% <46.15%> (-0.06%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/base.py 96.42% <84.61%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.75% <0%> (-0.1%) ⬇️
pandas/plotting/_converter.py 65.2% <0%> (+1.81%) ⬆️

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 aed9b92...f11a40e. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 15, 2017

Codecov Report

Merging #17878 into master will increase coverage by 0.01%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17878      +/-   ##
==========================================
+ Coverage   91.23%   91.24%   +0.01%     
==========================================
  Files         163      163              
  Lines       50102    50111       +9     
==========================================
+ Hits        45712    45726      +14     
+ Misses       4390     4385       -5
Flag Coverage Δ
#multiple 89.06% <84.61%> (+0.02%) ⬆️
#single 40.31% <46.15%> (-0.06%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/base.py 96.42% <84.61%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/common.py 91.18% <0%> (-1.63%) ⬇️
pandas/core/frame.py 97.75% <0%> (-0.1%) ⬇️
pandas/core/reshape/pivot.py 96.35% <0%> (-0.03%) ⬇️
pandas/core/series.py 94.99% <0%> (ø) ⬆️
pandas/core/dtypes/dtypes.py 95.14% <0%> (ø) ⬆️
pandas/core/reshape/concat.py 97.58% <0%> (ø) ⬆️
pandas/core/groupby.py 92.03% <0%> (+0.05%) ⬆️
pandas/core/generic.py 92.53% <0%> (+0.33%) ⬆️
... and 1 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 aed9b92...5039133. Read the comment docs.

@@ -896,6 +896,33 @@ def test_difference(self):
assert len(result) == 0
assert result.name == first.name

def test_difference_sorting_false(self):
Copy link
Member

Choose a reason for hiding this comment

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

  • Reference issue number
  • Will need test cases that cause your code to issue the warnings you added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gfyoung any changes required?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gfyoung gfyoung added Enhancement Indexing Related to indexing on series/frames, not to indexes themselves labels Oct 16, 2017
@pep8speaks
Copy link

pep8speaks commented Oct 18, 2017

Hello @Giftlin! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on October 18, 2017 at 10:23 Hours UTC

union = Index([]).union(first, sort=False)
assert union is first

# preserve names
Copy link
Contributor

Choose a reason for hiding this comment

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

pls dont' repeat code like this, use parametrize instead.

@jreback
Copy link
Contributor

jreback commented Nov 23, 2017

can you rebase and update according to comments

@Licht-T
Copy link
Contributor

Licht-T commented Dec 8, 2017

@jreback Can I take over this issue?

@gfyoung
Copy link
Member

gfyoung commented Dec 8, 2017

@Licht-T : Feel free to write your own patch and submit as a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Indexing Related to indexing on series/frames, not to indexes themselves
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: sort=bool keyword argument for index.difference
5 participants