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: DataFrame.apply not adding a frequency if freq=None (#22150) #22561

Merged
merged 6 commits into from
Sep 18, 2018

Conversation

HannahFerch
Copy link
Contributor

@HannahFerch HannahFerch commented Sep 1, 2018

@gfyoung gfyoung added Frequency DateOffsets Apply Apply, Aggregate, Transform, Map labels Sep 1, 2018
@gfyoung
Copy link
Member

gfyoung commented Sep 1, 2018

@HannahFerch : Good start! Will need a whatsnew and test(s) as well.

@codecov
Copy link

codecov bot commented Sep 1, 2018

Codecov Report

Merging #22561 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22561      +/-   ##
==========================================
- Coverage   92.17%   92.17%   -0.01%     
==========================================
  Files         169      169              
  Lines       50708    50706       -2     
==========================================
- Hits        46740    46738       -2     
  Misses       3968     3968
Flag Coverage Δ
#multiple 90.58% <ø> (-0.01%) ⬇️
#single 42.35% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/datetimes.py 95.66% <ø> (-0.02%) ⬇️

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 0976e12...13c4fa9. Read the comment docs.

@jreback
Copy link
Contributor

jreback commented Sep 4, 2018

this needs tests. you can simply add the OP hypthoesis test (and the minimal example if needed). pls add a whatsnew note in 0.24

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.

comments

@HannahFerch
Copy link
Contributor Author

Thanks for the feedback.
@jreback Not sure what is meant by the OP hypothesis test. I would add a test similar to the example @mroeschke shared in #22150 - would that be sufficient?

@mroeschke
Copy link
Member

@HannahFerch ideally if you could include this test #22150 (comment) in our testing suite, that'd be great.

I think pandas/tests/frame/test_apply.py should be an apt location for the test.

@pep8speaks
Copy link

Hello @HannahFerch! Thanks for updating the PR.

@HannahFerch
Copy link
Contributor Author

@mroeschke Thanks, have tried to do as you suggested

@@ -628,7 +628,7 @@ Datetimelike
- Bug in :meth:`DataFrame.eq` comparison against ``NaT`` incorrectly returning ``True`` or ``NaN`` (:issue:`15697`,:issue:`22163`)
- Bug in :class:`DatetimeIndex` subtraction that incorrectly failed to raise `OverflowError` (:issue:`22492`, :issue:`22508`)
- Bug in :class:`DatetimeIndex` incorrectly allowing indexing with ``Timedelta`` object (:issue:`20464`)
-
- Bug in :class:`DatetimeIndex` where frequency was being set if original frequency was 'None' (:issue:`22150`)
Copy link
Member

Choose a reason for hiding this comment

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

Nit pick: Could you replace the quotes around None with double backticks? Like how Timedelta is in the line above.

df.apply(lambda x: x)
assert index.freq == original.freq

def test_frequency_is_original_example(self):
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this test anymore since it should be covered by the test above.

@HannahFerch
Copy link
Contributor Author

@mroeschke Done. Please let me know if anything else is needed from my side.

Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

LGTM. @jreback

@jreback jreback added this to the 0.24.0 milestone Sep 18, 2018
@jreback jreback merged commit 33f38b9 into pandas-dev:master Sep 18, 2018
@jreback
Copy link
Contributor

jreback commented Sep 18, 2018

thanks @HannahFerch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Apply Apply, Aggregate, Transform, Map Frequency DateOffsets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataFrame.apply adds a frequency to a freq=None DatetimeIndex as a side-effect
5 participants