Skip to content

Fix frame_or_series.asfreq() dropping rows on unordered indices #40384

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

Merged
merged 8 commits into from
Mar 23, 2021

Conversation

nmay231
Copy link
Contributor

@nmay231 nmay231 commented Mar 11, 2021

This prevents frame_or_series.asfreq() from dropping rows/items if the index is not sorted.

This seemed like a relatively easy fix, and it should probably be done sooner than later. However, there wasn't much discussion in the issue so I still have some questions:

  1. How should we respond to unsorted indices? For now, I went ahead with option a.
    a. Silently "sort" the index before reindexing. Or we could
    b. Raise a warning before "sorting." Or
    c. Raise an error.
  2. If it should be in the whatsnew, which version would it be added to?
  3. I was thinking of converting the asfreq tests from class-based to functions. Can I do that in this PR or should I do that in another one?

@simonjayhawkins simonjayhawkins added Bug Frequency DateOffsets Datetime Datetime data dtype labels Mar 14, 2021
@jreback
Copy link
Contributor

jreback commented Mar 15, 2021

also pls add a whatsnew note, 1.3. bug fixes, resample section

@nmay231
Copy link
Contributor Author

nmay231 commented Mar 18, 2021

@jreback Look good?

@nmay231 nmay231 requested a review from jreback March 21, 2021 05:53
@jreback jreback added this to the 1.3 milestone Mar 21, 2021
@nmay231
Copy link
Contributor Author

nmay231 commented Mar 22, 2021

@jreback I just changed the test to resemble the previous test in format. Hopefully, that makes its intention more clear?

@nmay231 nmay231 requested a review from jreback March 22, 2021 00:50
expected = result.copy().reindex(sorted(index))

result = result.asfreq("D")
expected = expected.asfreq("D")
Copy link
Contributor

Choose a reason for hiding this comment

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

right after L101, do this instead
expected.index = expected.index._with_freq('D')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

@nmay231 nmay231 Mar 22, 2021

Choose a reason for hiding this comment

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

I couldn't use ._with_freq('D'); it failed with an assertion error because it expects None or 'infer'.

I used infer.

@nmay231 nmay231 requested a review from jreback March 22, 2021 23:35
@jreback jreback merged commit 2cea420 into pandas-dev:master Mar 23, 2021
@jreback
Copy link
Contributor

jreback commented Mar 23, 2021

thanks @nmay231

@nmay231 nmay231 deleted the fix-asfreq branch March 23, 2021 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Datetime Datetime data dtype Frequency DateOffsets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: asfreq silently drops rows when index is not sorted
3 participants