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.

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

Bug groupby idxmin #25531

Merged
merged 15 commits into from Mar 30, 2019
Merged

Bug groupby idxmin #25531

merged 15 commits into from Mar 30, 2019

Conversation

rbenes
Copy link
Contributor

@rbenes rbenes commented Mar 4, 2019

closes #25444
closes #15306

  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25531      +/-   ##
==========================================
+ Coverage   91.75%   91.75%   +<.01%     
==========================================
  Files         173      173              
  Lines       52960    52962       +2     
==========================================
+ Hits        48595    48597       +2     
  Misses       4365     4365
Flag Coverage Δ
#multiple 90.33% <100%> (ø) ⬆️
#single 41.71% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/groupby/generic.py 87.03% <100%> (+0.03%) ⬆️

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 bd49d2f...538f8f5. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #25531 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25531      +/-   ##
==========================================
+ Coverage   91.26%   91.47%   +0.21%     
==========================================
  Files         172      173       +1     
  Lines       52965    52875      -90     
==========================================
+ Hits        48337    48367      +30     
+ Misses       4628     4508     -120
Flag Coverage Δ
#multiple 90.03% <100%> (+0.2%) ⬆️
#single 41.82% <12.5%> (+0.07%) ⬆️
Impacted Files Coverage Δ
pandas/core/groupby/generic.py 87.1% <100%> (+0.1%) ⬆️
pandas/core/groupby/base.py 91.83% <100%> (ø) ⬆️
pandas/io/feather_format.py 89.47% <0%> (-0.27%) ⬇️
pandas/core/computation/engines.py 88.7% <0%> (-0.18%) ⬇️
pandas/plotting/_tools.py 78.65% <0%> (-0.12%) ⬇️
pandas/io/packers.py 88.11% <0%> (-0.11%) ⬇️
pandas/core/panel.py 35.62% <0%> (-0.1%) ⬇️
pandas/io/excel/_xlrd.py 93.93% <0%> (-0.1%) ⬇️
pandas/io/formats/printing.py 85.34% <0%> (-0.08%) ⬇️
pandas/util/testing.py 89.3% <0%> (-0.05%) ⬇️
... and 55 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 8e54e55...28b5ab2. Read the comment docs.

pandas/core/groupby/generic.py Show resolved Hide resolved
pandas/tests/groupby/test_groupby.py Outdated Show resolved Hide resolved
pandas/tests/groupby/test_groupby.py Outdated Show resolved Hide resolved
pandas/tests/groupby/test_groupby.py Outdated Show resolved Hide resolved
pandas/tests/groupby/test_groupby.py Outdated Show resolved Hide resolved
Copy link
Member

@WillAyd WillAyd 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 whatsnew for 0.24.2?

pandas/core/groupby/generic.py Show resolved Hide resolved
pandas/tests/groupby/test_function.py Outdated Show resolved Hide resolved
pandas/tests/groupby/test_function.py Outdated Show resolved Hide resolved
@jreback
Copy link
Contributor

jreback commented Mar 10, 2019

can you update

@rbenes
Copy link
Contributor Author

rbenes commented Mar 11, 2019

can you update

I do not know, how to deal with one of your previous comment that says "this should already be done in _try_cast" . I didn't understand it clearly. I do not know if it should be already there (without my changes) and I should only debug it, or if I should move the logic (if self._transform_should_cast(func)) into _try_cast function (and potentially rename _transform_should_cast to generic name _should_cast as @WillAyd reqested)...
Thanks in advance.

@WillAyd
Copy link
Member

WillAyd commented Mar 14, 2019

should move the logic (if self._transform_should_cast(func)) into _try_cast

I think this would be most logical

@rbenes
Copy link
Contributor Author

rbenes commented Mar 14, 2019

should move the logic (if self._transform_should_cast(func)) into _try_cast

I think this would be most logical

Ouuu, I tried to do it but I found another problem - in _try_cast you do not have func parameter. So you are not able to decide if cast or not inside _try_cast...

doc/source/whatsnew/v0.24.2.rst Outdated Show resolved Hide resolved
doc/source/whatsnew/v0.24.2.rst Outdated Show resolved Hide resolved
doc/source/whatsnew/v0.24.2.rst Outdated Show resolved Hide resolved
@jreback jreback added this to the 0.25.0 milestone Mar 19, 2019
@jreback
Copy link
Contributor

jreback commented Mar 19, 2019

@rbenes can you have a look thru groupby / datetime issues, I am almost sure this is a duplicate (which this PR would solve)

@rbenes
Copy link
Contributor Author

rbenes commented Mar 20, 2019

@rbenes can you have a look thru groupby / datetime issues, I am almost sure this is a duplicate (which this PR would solve)

I found these, but none of them is very close to this PR:

already closed - #19200
"similar", but not solved in my PR - #17035
fixed even on master - #15306

@WillAyd
Copy link
Member

WillAyd commented Mar 22, 2019

@rbenes the last of those issues still persists on me for master - are you sure this doesn't solve that as well?

@jreback
Copy link
Contributor

jreback commented Mar 22, 2019

@rbenes yeah this looks ok, but I agree here that #15306 should be solved by this.

@rbenes
Copy link
Contributor Author

rbenes commented Mar 22, 2019

@rbenes yeah this looks ok, but I agree here that #15306 should be solved by this.

@WillAyd @jreback yes guys, you both were right.
here is output:

master

(pandas) ➜  pandas git:(master) ✗ python my_test.py
                price  week_id
2011-01-01  10.050562       52
2011-01-02  10.550513       52
2011-01-03   9.554604        1
2011-01-04  10.248203        1
2011-01-05   9.829901        1
2011-01-06   8.245324        1
2011-01-07   7.597617        1
2011-01-08   8.196192        1
2011-01-09   8.528442        1
2011-01-10   7.380966        2
2011-01-11   7.999635        2
2011-01-12   7.911648        2
2011-01-13   8.336721        2
2011-01-14   8.668974        2
2011-01-15   7.512158        2

2011-01-01    1.293926e+18
2011-01-02    1.293926e+18
2011-01-03    1.294099e+18
2011-01-04    1.294099e+18
2011-01-05    1.294099e+18
2011-01-06    1.294099e+18
2011-01-07    1.294099e+18
2011-01-08    1.294099e+18
2011-01-09    1.294099e+18
2011-01-10    1.294963e+18
2011-01-11    1.294963e+18
2011-01-12    1.294963e+18
2011-01-13    1.294963e+18
2011-01-14    1.294963e+18
2011-01-15    1.294963e+18
Freq: D, Name: price, dtype: float64
week_id
1    2011-01-04
2    2011-01-14
52   2011-01-02

after PR

(pandas) ➜  pandas git:(bug_groupby_idxmin) ✗ python my_test.py
                price  week_id
2011-01-01  10.050562       52
2011-01-02  10.550513       52
2011-01-03   9.554604        1
2011-01-04  10.248203        1
2011-01-05   9.829901        1
2011-01-06   8.245324        1
2011-01-07   7.597617        1
2011-01-08   8.196192        1
2011-01-09   8.528442        1
2011-01-10   7.380966        2
2011-01-11   7.999635        2
2011-01-12   7.911648        2
2011-01-13   8.336721        2
2011-01-14   8.668974        2
2011-01-15   7.512158        2

2011-01-01   2011-01-02
2011-01-02   2011-01-02
2011-01-03   2011-01-04
2011-01-04   2011-01-04
2011-01-05   2011-01-04
2011-01-06   2011-01-04
2011-01-07   2011-01-04
2011-01-08   2011-01-04
2011-01-09   2011-01-04
2011-01-10   2011-01-14
2011-01-11   2011-01-14
2011-01-12   2011-01-14
2011-01-13   2011-01-14
2011-01-14   2011-01-14
2011-01-15   2011-01-14
Freq: D, Name: price, dtype: datetime64[ns]
week_id
1    2011-01-04
2    2011-01-14
52   2011-01-02

@jreback
Copy link
Contributor

jreback commented Mar 22, 2019

@rbenes great. can you add that as an additional test (and add to the whatsnew note that issue)

pandas/tests/groupby/test_transform.py Outdated Show resolved Hide resolved
doc/source/whatsnew/v0.25.0.rst Outdated Show resolved Hide resolved
@jreback jreback merged commit 3de9137 into pandas-dev:master Mar 30, 2019
@jreback
Copy link
Contributor

jreback commented Mar 30, 2019

thanks @rbenes

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

Successfully merging this pull request may close these issues.

groupby.idxmin() returns datetime values for datatime columns Groupby transform idxmax return floats
4 participants