BUG: Fixed encoding of pd.NA with to_json #31748
Conversation
| @@ -25,8 +25,9 @@ Fixed regressions | |||
| Bug fixes | |||
| ~~~~~~~~~ | |||
|
|
|||
| - | |||
| - | |||
| **I/O** | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
@MarcoGorelli I've updated that to the correct format.
There was a problem hiding this comment.
The original was actually fine, to be consistent with the v1.0.1 file
(sorry for the conflicting comments)
There was a problem hiding this comment.
My mistake, sorry, I'd taken that from v1.1.0 where we go back to
I/O
^^^
, will be more careful in the future
There was a problem hiding this comment.
@MarcoGorelli no problem! It's also inconsistent between the different files .. (for the bug fix releases, the list of bugs is much smaller, so less worth it to make subsections. At least, that is what we just did for the v1.0.1 file)
There was a problem hiding this comment.
Reverted to original format
pandas/tests/io/json/test_pandas.py
Outdated
| @@ -1671,3 +1671,15 @@ def test_to_s3(self, s3_resource): | |||
| assert target_file in ( | |||
| obj.key for obj in s3_resource.Bucket("pandas-test").objects.all() | |||
| ) | |||
|
|
|||
| @pytest.mark.parametrize( | |||
There was a problem hiding this comment.
I think the comment around parametrization got lost in GH comments but in any case would rather parametrize on nulls_fixture + pd.NA if we don't think we can add pd.NA to the former for now
I don't think the current parametrization is super useful, so OK to just take the first case and put it in the body of the test
There was a problem hiding this comment.
parametrize on nulls_fixture + pd.NA
+1
There was a problem hiding this comment.
Couldn't find a way to add pd.NA to a pytest fixture without recreating the contents of the fixture entirely, so I've added them as separate tests. Let me know if you were thinking of something else.
Note that pd.NaT is the only object that doesn't use its string representation when encoded from a label.
|
@WillAyd I've ran the full asv suite, and then re-ran any asv tests that had a ratio >1.1 There are no significant changes from this PR. Let me know if you want any more information or results files. |
WillAyd
added
IO JSON
There was a problem hiding this comment.
@WillAyd I've ran the full asv suite, and then re-ran any asv tests that had a ratio >1.1
I think I understand though ideally shouldn't have gotten anything over 1.1 in the first place. Did you run the entire suite or just asv continuous upstream/master HEAD -b io.json.ToJSON?
The latter is all you needed here, so noting for future reference.
Otherwise small nit / question for @simonjayhawkins . Can get this in soon
| result = pd.DataFrame({pd.NA: ["a"]}).to_json() | ||
| assert result == '{"<NA>":{"0":"a"}}' | ||
|
|
||
| def test_json_pandas_nulls(self, nulls_fixture): |
There was a problem hiding this comment.
@simonjayhawkins do you know of an easy way to add pd.NA to nulls_fixture inline during injection? Or would adding another fixture on top be the only way?
There was a problem hiding this comment.
since we already have two nulls_fixtures (nulls_fixture and unique_nulls_fixture), I think adding another could be the simplest way for now.
I think, in general, simpler to skip than to add a extra parameter (although I've not kept up with the latest pytest releases).
There is not a huge number of tests using the nulls_fixture and maybe tests that do should also test for pandas.NA, either now of in the future. So an alternative could be to add pd.NA to the existing nulls fixture and either skip of xfail.
pandas/tests/io/json/test_pandas.py
Outdated
| result = pd.DataFrame([[pd.NA]]).to_json() | ||
| assert result == '{"0":{"0":null}}' | ||
|
|
||
| def test_json_pandas_na_label(self): |
There was a problem hiding this comment.
@ArtificialQualia can just remove the label tests for now; not touched by your test and I don't think these expectations are necessarily correct, so let's leave that to a different exercise
There was a problem hiding this comment.
I've removed the label tests. Let me know what the final decision is above about the fixtures.
|
@WillAyd I ran the entire suite. Whenever I run the entire suite I always get false positives, so I rerun anything that shows up on the first go-around. There are no significant performance differences in io.json.ToJSON (or anywhere else). |
|
@ArtificialQualia Thanks a lot! |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon! If these instruction are inaccurate, feel free to suggest an improvement. |
Co-authored-by: Kendall Masse <[email protected]>
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff