Skip to content

implement fillna from 24024, with fixes and tests #24536

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 1, 2019
Merged

implement fillna from 24024, with fixes and tests #24536

merged 2 commits into from
Jan 1, 2019

Conversation

jbrockmendel
Copy link
Member

cc @jreback @TomAugspurger

couple of issues with fillna needed sorting out

  • The DTA version was operating in-place (fixed+tested)
  • The TDA version would raise because it wasn't supported in core.missing (fixed+tested)
  • The DTA[tz] version would incorrectly re-localize using the existing constructors, i.e. was dependent on the constructor changes in 24024. With the edits here it is correct regardless of whether the constructor is changed.

@jbrockmendel
Copy link
Member Author

kind of weird that Series[timedelta64].fillna(method='pad') works OK in master, will need to track down why it hadn't been supported directly in core.missing.pad_1d

@codecov
Copy link

codecov bot commented Jan 1, 2019

Codecov Report

Merging #24536 into master will decrease coverage by <.01%.
The diff coverage is 12.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24536      +/-   ##
==========================================
- Coverage   31.88%   31.87%   -0.01%     
==========================================
  Files         166      166              
  Lines       52427    52438      +11     
==========================================
  Hits        16714    16714              
- Misses      35713    35724      +11
Flag Coverage Δ
#multiple 30.27% <12.5%> (-0.01%) ⬇️
#single 31.87% <12.5%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/arrays/period.py 41.87% <ø> (+1.26%) ⬆️
pandas/core/missing.py 10.25% <0%> (-0.11%) ⬇️
pandas/core/arrays/datetimelike.py 37.11% <14.28%> (-1.25%) ⬇️

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 091cfbb...aa9f457. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 1, 2019 •
edited
Loading

Codecov Report

Merging #24536 into master will increase coverage by 60.44%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #24536       +/-   ##
===========================================
+ Coverage   31.88%   92.32%   +60.44%     
===========================================
  Files         166      166               
  Lines       52427    52440       +13     
===========================================
+ Hits        16714    48417    +31703     
+ Misses      35713     4023    -31690
Flag Coverage Δ
#multiple 90.75% <100%> (+60.46%) ⬆️
#single 43% <12.5%> (+11.12%) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/period.py 98.52% <ø> (+57.91%) ⬆️
pandas/core/arrays/datetimelike.py 96.61% <100%> (+58.24%) ⬆️
pandas/core/missing.py 92.3% <100%> (+81.94%) ⬆️
pandas/util/_test_decorators.py 90.54% <0%> (+4.05%) ⬆️
pandas/compat/__init__.py 57.91% <0%> (+9.65%) ⬆️
pandas/core/config_init.py 99.24% <0%> (+9.84%) ⬆️
pandas/core/reshape/util.py 100% <0%> (+11.53%) ⬆️
pandas/core/api.py 100% <0%> (+13.33%) ⬆️
pandas/compat/numpy/__init__.py 92.85% <0%> (+14.28%) ⬆️
pandas/io/formats/console.py 74.24% <0%> (+16.66%) ⬆️
... and 128 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 091cfbb...52e01ae. Read the comment docs.

@jreback jreback added Refactor Internal refactoring of code Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels Jan 1, 2019
jreback
jreback reviewed Jan 1, 2019
View reviewed changes
pandas/core/missing.py
@@ -481,6 +481,10 @@ def pad_1d(values, limit=None, mask=None, dtype=None):
_method = algos.pad_inplace_float64
elif values.dtype == np.object_:
_method = algos.pad_inplace_object
elif is_timedelta64_dtype(values):
# NaTs are treated identically to datetime64, so we can dispatch
Copy link
Contributor

Choose a reason for hiding this comment

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

is something failing? i agree this is probably the soln. but likely these are converted prior to calling this (to i8)

Copy link
Member Author

Choose a reason for hiding this comment

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

Presumably; I haven’t looked at the series code to see how it handles this. Enough core.missing mysteries have popped up today I’ll be giving them a close look after the RC

Copy link
Contributor

Choose a reason for hiding this comment

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

sure

@jbrockmendel
Copy link
Member Author

gentle ping

@jreback jreback added this to the 0.24.0 milestone Jan 1, 2019
@jreback jreback merged commit cf92230 into pandas-dev:master Jan 1, 2019
@jreback
Copy link
Contributor

jreback commented Jan 1, 2019

thanks

@jbrockmendel jbrockmendel deleted the fillna branch January 1, 2019 23:14
thoo added a commit to thoo/pandas that referenced this pull request Jan 2, 2019
@thoo
Merge remote-tracking branch 'upstream/master' into isort-io
39f549a 
* upstream/master:
  implement fillna from 24024, with fixes and tests (pandas-dev#24536)
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
@jbrockmendel @Pingviinituutti
implement fillna from 24024, with fixes and tests (pandas-dev#24536)
7b09641
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
@jbrockmendel @Pingviinituutti
implement fillna from 24024, with fixes and tests (pandas-dev#24536)
900cd03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback left review comments

Assignees
No one assigned
Labels
Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Refactor Internal refactoring of code
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.
2 participants
@jbrockmendel @jreback