Skip to content

BUG: to_period() freq was not infered #33406

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 15 commits into from
Apr 12, 2020

Conversation

ShaharNaveh
Copy link
Member

@ShaharNaveh ShaharNaveh commented Apr 8, 2020


When running pytest -q --cache-clear pandas/tests/ -k "period" some tests in pandas/tests/indexing/ are failing, but when running pytest -q --cache-clear pandas/tests/indexing/ all the tests are passing (on my local machine), so I figured and debug from here, thoughts?

@jbrockmendel
Copy link
Member

thoughts?

we cant rely on grepping to get all the relevant tests. i recommend pytest pandas/tests --skip-slow --skip-db, takes about 15 minutes for me.

Look at the failing tests, choose one and start executing it step-by-step in the REPL

off = "QS"
rng = pd.date_range("01-Jan-2012", periods=8, freq=off)

>>> prng = rng.to_period()   # <-- raises, so recurse into the `to_period` method

)

# Using simple filter because we are not checking for the warning here
warnings.simplefilter("ignore", UserWarning)
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the warning from? this would not be set like this
use assert_produces_warning(None):
warnings.simplefiler....

if you really need to

Copy link
Member Author

Choose a reason for hiding this comment

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

what is the warning from? this would not be set like this

The warning raises when a user is attempting to convert a date_range to period, if the date_range have a timezone it will drop it in the conversion, the warning let the user know about that:

if self.tz is not None:
warnings.warn(
"Converting to PeriodArray/Index representation "
"will drop timezone information.",
UserWarning,
)

use assert_produces_warning(None):
warnings.simplefiler....

if you really need to

Sure! will fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

i c, ok then

@jreback jreback added Frequency DateOffsets Period Period data type labels Apr 10, 2020
@jreback jreback added this to the 1.1 milestone Apr 10, 2020
@jreback
Copy link
Contributor

jreback commented Apr 10, 2020

looks good, can you add a whatsnew note in 1.1 in the datetime section for bug fixes. ping on green.

@ShaharNaveh ShaharNaveh requested a review from jreback April 11, 2020 14:39
@ShaharNaveh
Copy link
Member Author

ping @jreback

@jreback jreback merged commit 13dc13f into pandas-dev:master Apr 12, 2020
@jreback
Copy link
Contributor

jreback commented Apr 12, 2020

thanks @MomIsBestFriend

@ShaharNaveh ShaharNaveh deleted the BUG-DatetimeIndex-to_period branch April 13, 2020 08:48
# https://github.com/pandas-dev/pandas/issues/33358
if res is None:
base, stride = libfrequencies._base_and_stride(freq)
res = f"{stride}{base}"
Copy link
Member

Choose a reason for hiding this comment

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

@MomIsBestFriend why cant we just do res = freq here? (im trying to clean up usages of base_and_stride)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frequency DateOffsets Period Period data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DatetimeIndex.to_period with freq
3 participants