Skip to content

Conversation

ahcub
Copy link
Contributor

@ahcub ahcub commented Dec 6, 2018

@pep8speaks
Copy link

pep8speaks commented Dec 6, 2018

Hello @ahcub! Thanks for updating the PR.

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

Comment last updated on December 26, 2018 at 00:17 Hours UTC

@codecov
Copy link

codecov bot commented Dec 6, 2018

Codecov Report

Merging #24128 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24128      +/-   ##
==========================================
+ Coverage   92.29%    92.3%   +0.01%     
==========================================
  Files         162      163       +1     
  Lines       51832    51978     +146     
==========================================
+ Hits        47839    47980     +141     
- Misses       3993     3998       +5
Flag Coverage Δ
#multiple 90.71% <100%> (+0.01%) ⬆️
#single 42.99% <23.33%> (+0.01%) ⬆️
Impacted Files Coverage Δ
pandas/core/series.py 93.72% <ø> (+0.01%) ⬆️
pandas/core/arrays/base.py 97.59% <100%> (+0.17%) ⬆️
pandas/core/frame.py 96.91% <100%> (ø) ⬆️
pandas/core/generic.py 96.62% <100%> (-0.04%) ⬇️
pandas/core/arrays/period.py 98.48% <100%> (-0.02%) ⬇️
pandas/core/internals/blocks.py 93.81% <100%> (ø) ⬆️
pandas/core/groupby/groupby.py 96.8% <100%> (+0.15%) ⬆️
pandas/core/arrays/categorical.py 95.34% <100%> (+0.02%) ⬆️
pandas/core/arrays/sparse.py 92.1% <100%> (+0.01%) ⬆️
pandas/core/arrays/datetimes.py 97.6% <0%> (-0.64%) ⬇️
... and 25 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 f6cf7d9...a03cbf5. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 6, 2018

Codecov Report

Merging #24128 into master will decrease coverage by 49.17%.
The diff coverage is 58.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #24128       +/-   ##
===========================================
- Coverage    92.2%   43.03%   -49.18%     
===========================================
  Files         162      162               
  Lines       51701    51701               
===========================================
- Hits        47672    22247    -25425     
- Misses       4029    29454    +25425
Flag Coverage Δ
#multiple ?
#single 43.03% <58.33%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/frame.py 35.88% <100%> (-61.03%) ⬇️
pandas/core/internals/blocks.py 52.93% <50%> (-40.79%) ⬇️
pandas/core/series.py 49.32% <50%> (-44.38%) ⬇️
pandas/core/generic.py 39.65% <50%> (-57%) ⬇️
pandas/core/arrays/datetimelike.py 48.74% <66.66%> (-47.61%) ⬇️
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/core/categorical.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/tseries/converter.py 0% <0%> (-100%) ⬇️
... and 124 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 b841374...8b5cedb. Read the comment docs.

@ahcub ahcub changed the title ENH: fill_value argument for shift ENH: fill_value argument for shift #15486 Dec 6, 2018
@@ -320,6 +320,19 @@ def test_shift_categorical(self):
xp = DataFrame({'one': s1.shift(1), 'two': s2.shift(1)})
assert_frame_equal(rs, xp)

def test_shift_fill_value(self):
df = DataFrame(np.random.randnint(5), index=date_range('1/1/2000',
Copy link
Contributor

Choose a reason for hiding this comment

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

add the gh number as a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jreback jreback added Enhancement Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Dec 6, 2018
Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Probably need to add to Categorical, SparseArray. What about groupby?

@ahcub
Copy link
Contributor Author

ahcub commented Dec 7, 2018

added fill_value to shift implementations across different classes, according to the failures from tests

@ahcub
Copy link
Contributor Author

ahcub commented Dec 8, 2018

@jreback I made all the changes, except the Index subclasses, because I think Index doesn't need fill_value. it does the shift correctly already with the freq specified
can you review?

@mroeschke
Copy link
Member

One last linting error as well:

2018-12-25T18:31:36.1417763Z ERROR: /home/vsts/work/1/s/pandas/tests/arrays/sparse/test_array.py Imports are incorrectly sorted.
2018-12-25T18:31:36.1417924Z ERROR: /home/vsts/work/1/s/pandas/core/arrays/base.py Imports are incorrectly sorted.

@ahcub
Copy link
Contributor Author

ahcub commented Dec 25, 2018

used isort to fix those, hopefully no issues this time 🙏

@ahcub
Copy link
Contributor Author

ahcub commented Dec 25, 2018

just added the whatsnew note, not sure if it is ok
all the other checks were green

@ahcub
Copy link
Contributor Author

ahcub commented Dec 25, 2018

all green @jreback

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

2 tiny corrections. ping on green and will merge. thanks for being so responsive.

@@ -31,6 +31,7 @@ New features
- :func:`read_feather` now accepts ``columns`` as an argument, allowing the user to specify which columns should be read. (:issue:`24025`)
- :func:`DataFrame.to_html` now accepts ``render_links`` as an argument, allowing the user to generate HTML with links to any URLs that appear in the DataFrame.
See the :ref:`section on writing HTML <io.html>` in the IO docs for example usage. (:issue:`2679`)
- :meth:`DataFrame.shift` :meth:`Series.shift`, :meth:`ExtensionArray.shift`, :meth:`SparseArray.shift`, :meth:`Period.shift`, :meth:`GroupBy.shift`, :meth:`Categorical.shift`, :meth:`NDFrame.shift` and :meth:`Block.shift` now accept fill_value as an argument, allowing the user to specify a value which will be used instead of NA/NaT in the empty periods
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use double backticks on fill_value and add a reference to the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@@ -9,6 +9,8 @@
from pandas.compat import range
import pandas.util._test_decorators as td

from pandas.core.dtypes.missing import isna
Copy link
Contributor

Choose a reason for hiding this comment

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

in tests just
from pandas import isna

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@jreback jreback added this to the 0.24.0 milestone Dec 25, 2018
@jreback
Copy link
Contributor

jreback commented Dec 26, 2018

@ahcub lgtm. ping on green.

@ahcub
Copy link
Contributor Author

ahcub commented Dec 26, 2018

all green @jreback

@jreback jreback merged commit bf31c04 into pandas-dev:master Dec 26, 2018
@jreback
Copy link
Contributor

jreback commented Dec 26, 2018

thanks @ahcub

this was a pretty involved change, thanks for sticking with it!

@ahcub
Copy link
Contributor Author

ahcub commented Dec 26, 2018

thank you for all the support!

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: fill_value argument for shift
5 participants