API/BUG: Make to_json index= arg consistent with orient arg by dshemetov · Pull Request #52143 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation19 Commits12 Checks0 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
Updates to_json with less surprising behavior with the index arg:
- split and table allow index=True/False (as before)
- records and values only allow index=False
- index and columns only allow index=True
- raise for contradictions in the latter two and update messages
- add test
- closes DataFrame.to_json silently ignores index parameter for most orients. #25513, closes BUG: DataFrame.to_json ignores index if it is repeated in the DataFrame. #37600
- Tests added and passed if fixing a bug or adding a new feature
- All code checks passed.
- Added type annotations to new arguments/methods/functions.
- Added an entry in the latest
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.
- split and table allow index=True/False
- records and values only allow index=False
- index and columns only allow index=True
- raise for contradictions in the latter two
- see pandas-dev#25513
Hey @WillAyd, this is a PR for your request here. Let me know who I can tag for review.
| Whether to include the index values in the JSON string. Not |
|---|
| including the index (``index=False``) is only supported when |
| orient is 'split' or 'table'. |
| index : bool or None, default None |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be easier to just say that the index is only used in split, index, column and table orients. Of those formats, index and column cannot be False.
You are kind of doing this now but I think in a way that is a bit more confusing. If you structure the commentary and code this will I think will help simplify the logic
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some changes, let me know what you think!
| "'index=False' is only valid when 'orient' is 'split', 'table', " |
|---|
| "'records', or 'values'." |
| ) |
| elif index and orient in ["records", "values"]: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The orient check is the same here as on L149 - can these not be consolidated?
| indent: int = 0, |
|---|
| storage_options: StorageOptions = None, |
| mode: Literal["a", "w"] = "w", |
| ) -> str | None: |
| if not index and orient not in ["split", "table"]: |
| if index is None and orient in ["records", "values"]: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this branch necessary? Or can you just change L151 to set it to True for appropriate orients?
Consolidated the checks up front, hopefully this is clearer
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.
Waiting on review, friendly ping @WillAyd
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think looks good. This could use a whatsnew note for 2.1. @mroeschke any thoughts on your end?
Looks good. Can you add the 2.1 whatsnew note?
Took a stab at it, lmk if the whatsnew note looks good (never done one before)
sort whatsnew entries alphabetically....................................................................Failed
- hook id: sort-whatsnew-items
- duration: 0.05s
- exit code: 1
- files were modified by this hook
pre-commit hook(s) made changes.
If you are seeing this message in CI, reproduce locally with: `pre-commit run --all-files`.
To run `pre-commit` as part of git workflow, use `pre-commit install`.
All changes made by hooks:
diff --git a/doc/source/whatsnew/v2.1.0.rst b/doc/source/whatsnew/v2.1.0.rst
index 56b7327..e0f417e 100644
--- a/doc/source/whatsnew/v2.1.0.rst
+++ b/doc/source/whatsnew/v2.1.0.rst
@@ -96,9 +96,9 @@ Other enhancements
- Let :meth:`DataFrame.to_feather` accept a non-default :class:`Index` and non-string column names (:issue:`51787`)
- Performance improvement in :func:`read_csv` (:issue:`52632`) with ``engine="c"``
- :meth:`Categorical.from_codes` has gotten a ``validate`` parameter (:issue:`50975`)
+- Improved error messaging when using :meth:`DataFrame.to_json` with incompatible ``index`` and ``orient`` arguments (:issue:`52143`)
- Performance improvement in :func:`concat` with homogeneous ``np.float64`` or ``np.float32`` dtypes (:issue:`52685`)
- Performance improvement in :meth:`DataFrame.filter` when ``items`` is given (:issue:`52941`)
-- Improved error messaging when using :meth:`DataFrame.to_json` with incompatible ``index`` and ``orient`` arguments (:issue:`52143`)
Rylie-W pushed a commit to Rylie-W/pandas that referenced this pull request
- API/BUG: Make to_json index= consistent with orient
- split and table allow index=True/False
- records and values only allow index=False
- index and columns only allow index=True
- raise for contradictions in the latter two
- see pandas-dev#25513
style: lint
style: make mypy happy
review: simplify
review: clarify and consolidate branches
style: add explainer comment
doc: change error message in _json
docs: update whatsnew 2.1.0
docs: sort whatsnew
Daquisu pushed a commit to Daquisu/pandas that referenced this pull request
- API/BUG: Make to_json index= consistent with orient
- split and table allow index=True/False
- records and values only allow index=False
- index and columns only allow index=True
- raise for contradictions in the latter two
- see pandas-dev#25513
style: lint
style: make mypy happy
review: simplify
review: clarify and consolidate branches
style: add explainer comment
doc: change error message in _json
docs: update whatsnew 2.1.0
docs: sort whatsnew
ajnelson-nist added a commit to casework/CASE-Utilities-Python that referenced this pull request
A follow-on patch will regenerate Make-managed files.
References:
Signed-off-by: Alex Nelson alexander.nelson@nist.gov
Hey all, for what it's worth I think I'm running into a segfault in release 2.0.3 that was fixed by this PR. Here is my bug report. If it wasn't this PR, it was something that was fixed between 2.0.3 and 2.1.0
This PR only disallowed certain combinations of arguments to to_json(), so I don't think it's the cause of the segfault in your report. I don't have any ideas that can help you off the top of my head, but I'll let you know if I do. Best of luck!