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: #12624 where Panel.fillna() ignores inplace=True #12633

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@Xbar
Copy link
Contributor

commented Mar 15, 2016

Added if condition for fillna when ndim==3 (for Panel) Issue: #12624

@jreback

View changes

pandas/core/generic.py Outdated
if inplace:
new_obj = self._constructor.from_dict(result).__finalize__(self)
self._update_inplace(new_obj._data)
return

This comment has been minimized.

Copy link
@jreback

jreback Mar 15, 2016

Contributor

just do an else here

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2016

this is duped by #12630 but your soln is nicer. Needs tests.

@jreback jreback changed the title Inplace fillna for ndim==3 BUG: #12624 where Panel.fillna() ignores inplace=True Mar 15, 2016

@jreback jreback referenced this pull request Mar 15, 2016

Closed

BUG: #12624 where Panel.fillna() ignores inplace=True #12630

4 of 4 tasks complete
@Xbar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2016

Added test case in test_panel.py: test_fillna
Using data provided in issue: #12624

@jreback

View changes

pandas/tests/test_panel.py Outdated
items=['a', 'b'], minor_axis=['x', 'y'])
p2 = pd.Panel([[[0, 1], [2, 1]], [[10, 11], [12, 11]]],
items=['a', 'b'], minor_axis=['x', 'y'])
p1.fillna(method='ffill', inplace=True)

This comment has been minimized.

Copy link
@jreback

jreback Mar 15, 2016

Contributor

rename p2 -> expected, drop the pd.

This comment has been minimized.

Copy link
@jreback

jreback Mar 15, 2016

Contributor

do this in a loop with ffill, bfill

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2016

pls add a whatsnew note as well.

@Xbar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2016

Test case changes:

  1. Add test case for Panel.fillna, method=‘bfill’
  2. Change variable name of expected result to “expected”.
  3. Fix discrepancy in dtype by forcing both Panel in this test case to have
    dtype=np.float64

Style changes in generic.py:
Added "else" in fillna when ndim==3. Better readability.

@jreback

View changes

pandas/core/generic.py Outdated
if inplace:
new_obj = self._constructor.from_dict(result).__finalize__(self)
self._update_inplace(new_obj._data)
return

This comment has been minimized.

Copy link
@jreback

jreback Mar 15, 2016

Contributor

you don't need this return

@jreback

View changes

pandas/core/generic.py Outdated
new_obj = self._constructor.from_dict(result).__finalize__(self)
self._update_inplace(new_obj._data)
return
else:

This comment has been minimized.

Copy link
@jreback

jreback Mar 15, 2016

Contributor

do something like:

result = self._constructor.from_dict(result).__finalize__(self)
if inplace:
    self._update_inplace(result._data)
else:
   return result

This comment has been minimized.

Copy link
@Xbar

Xbar Mar 16, 2016

Author Contributor

return is necessary, because otherwise it will continue to run to the default code for "2d or less".
But I can refactor this part so that all changes to _data will be put into "new_data" and handled by downstream code.

This comment has been minimized.

Copy link
@Xbar

Xbar Mar 16, 2016

Author Contributor

Maybe a silly question, but where should I put the whatsnew note?

This comment has been minimized.

Copy link
@jreback

jreback Mar 16, 2016

Contributor

oh i c. ok then. whatsnew in doc/source/whatsnew/v0.18.1.txt

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2016

some small changes. pls add a whatsnew note.

@Xbar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2016

  1. Updated whatsnew.
  2. Style changes in generic.py.
    -Removed return.
    -Reduced if branches when ndim==3.
    -One "if" for inplace at the end of the method. Do not judge inplace twice, unless method is not implemented for some special cases.
@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

ok looks nice. if you can squash your commits would be great. pls ping on green. (not you had some PEP errors in the prior runs which cause things to fail).
git diff master | flake8 --diff to check/fix.

@jreback jreback added this to the 0.18.1 milestone Mar 16, 2016

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

pls also rebase on master: git rebase -i master (this will squash as well)

@Xbar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2016

Thanks for the reminder!
Sorry for making so many commits. I'm new to git and version control.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

no, making lots of commits is fine (and good!).

At the end we just like to squash them to a single one.

@codecov-io

This comment has been minimized.

Copy link

commented Mar 16, 2016

Current coverage is 88.64%

Merging #12633 into master will not affect coverage as of cb49b07

@@            master   #12633   diff @@
=======================================
  Files          275      275       
  Stmts       133688   133696     +8
  Branches         0        0       
  Methods          0        0       
=======================================
+ Hit         118511   118519     +8
  Partial          0        0       
  Missed       15177    15177       

Review entire Coverage Diff as of cb49b07

Powered by Codecov. Updated on successful CI builds.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2016

this needs to be squashed/rebased on master.

@Xbar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2016

I'm not sure if I'm doing it right. I'm running "git rebase -i 648b0f6" and squash all commits I made

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2016

git rebase -i master, then once squashed git push yourremote thisbranch -f

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2016

FYI, you don't want to git merge master normally, instead git pull --rebase. This is the workflow that we use here.

@Xbar Xbar force-pushed the Xbar:master branch Mar 17, 2016

@Xbar Xbar force-pushed the Xbar:master branch to cfb3882 Mar 17, 2016

@Xbar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2016

I finally got the idea here. Thanks!
Now it's just 1 commit.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2016

ok great. ping when green.

@@ -89,46 +89,17 @@ Bug Fixes
~~~~~~~~~

- Bug in ``Period`` and ``PeriodIndex`` creation raises ``KeyError`` if ``freq="Minute"`` is specified. Note that "Minute" freq is deprecated in v0.17.0, and recommended to use ``freq="T"`` instead (:issue:`11854`)
- Bug in ``Series`` construction with ``Categorical`` and ``dtype='category'`` is specified (:issue:`12574`)


This comment has been minimized.

Copy link
@jreback

jreback Mar 17, 2016

Contributor

in the future, don't take blank lines the whatsnew, just make your changes. I put these in because then we don't have conflicts with multiple PR's.

This comment has been minimized.

Copy link
@Xbar

Xbar Mar 17, 2016

Author Contributor

I see. git is telling me conflicts in whatsnew and I got confused.

@Xbar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2016

All checks passed, ready to merge.

@jreback jreback closed this in ab359c1 Mar 17, 2016

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2016

thanks @Xbar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.