BUG: conversion a JSON field descriptor into pandas type for deprecated offsets frequency 'M' #55581
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
natmokval
changed the title
mroeschke
added
IO JSON
MarcoGorelli
requested changes
Oct 20, 2023
pandas/io/json/_table_schema.py
Outdated
Comment on lines
211
to
212
| # GH#9586 rename frequency M to ME for offsets | ||
| field["freq"] = freq_to_period_freqstr(1, field["freq"]) |
There was a problem hiding this comment.
what if field['freq'] is '2M'?
There was a problem hiding this comment.
thanks, you are right, it doesn't work for '2M'. I corrected the definition of convert_json_field_to_pandas_type and updated my test.
| # GH#9586 | ||
| df = DataFrame( | ||
| {"ints": [1, 2]}, | ||
| index=pd.PeriodIndex(["2011-01", "2011-08"], freq="2M"), |
There was a problem hiding this comment.
could we check it works for both '2M' and 'M'?
There was a problem hiding this comment.
yeah, sure, I added parameterization to the test, for 'M' and '2M'.
MarcoGorelli
requested changes
Oct 23, 2023
pandas/io/json/_table_schema.py
Outdated
Comment on lines
212
to
215
| # GH#9586 rename frequency M to ME for offsets | ||
| freq_name = re.split("[0-9]*", field["freq"], maxsplit=1)[1] | ||
| freq_n = field["freq"][: field["freq"].index(freq_name)] | ||
| freq = freq_to_period_freqstr(freq_n, freq_name) |
There was a problem hiding this comment.
having a regular expression here looks a bit complex - would it work to do
offset = to_offset(field['freq'])
freq_n, freq_name = offset.n, offset.name
freq = freq_to_period_freqstr(freq_n, freq_name)instead?
There was a problem hiding this comment.
thank you, it works very well. I replaced the regular expression with to_offset and updated my PR.
3 tasks
MarcoGorelli
approved these changes
Oct 24, 2023
|
Thank you @MarcoGorelli for reviewing this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
xref #52064
when converting a JSON field descriptor into its corresponding pandas type we don't check if we have deprecated for offsets frequency "M".
corrected the definition of
convert_json_field_to_pandas_type, added a test.