-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Fix to_json when converting Period column #32665
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: Fix to_json when converting Period column #32665
Conversation
Fix GH31917. AttributeError when trying to access frequency string directly on Series
Can you add a test for this? @jbrockmendel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a test replicating the OP & a whatsnew note (bug fixes in 1.1 / IO)
Looks like a merge conflict in the whatsnew - can you fix and add a test case in pandas/tests/io/json/test_pandas.py? |
OK, I'll do it, but first I need to figure it out how to make a test. Have to read more carefully the guide. |
@colonesej you should just be able to place a test in pandas/tests/io/json/test_pandas.py for this (and can look at other examples there too) Can you fix up merge conflict here as well? |
Hello @colonesej! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-06-18 16:09:56 UTC |
I made this roundtrip test completely based on the one for a time series with a DatetimeIndex. Some points:
To illustrate, using DatetimeIndex:
PeriodIndex:
|
@@ -473,6 +473,15 @@ def _create_series(index): | |||
} | |||
|
|||
|
|||
@pytest.fixture | |||
def period_series(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd did we settle on a naming scheme for these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable; we already have a datetime_series so this follows that pattern
def test_series_roundtrip_periodseries(self, orient, period_series): | ||
# GH32665: Fix to_json when converting Period column/series | ||
data = period_series.to_json(orient=orient) | ||
result = pd.read_json(data, typ="series", orient=orient) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue affects reading to a DataFrame as well right? If so can you test that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I can do that.
The point with I don't know what should be the behaviour here:
I made this other test with a frame column with |
@colonesej can you merge master and try to get green? |
Yes, but you mean, make the roundtrip tests I've made pass? I could have some help about how Or could make a different test, testing based on some expected output template. Will not solve the roundtrip issue but will make it green. What is preferable? |
If there’s one orient that doesn’t work you can pytest.skip it for now - check some of the other tests in the module for examples |
@colonesej is this PR still active? |
AttributeError when trying to access frequency string
freqstr
directly on Series ofPeriod
type.Modified to get it through
dtype
of Series, which is known to bePeriod
.to_json
of Series with period dtype results in AttributeError #31917to_json
of Series with period dtype results in AttributeError #31917black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff