Deprecate Aliases as orient Argument in DataFrame.to_dict #32516

elmonsomiat · 2020-03-07T12:53:26Z

[ x] closes df.to_dict() accepts orient which are not in the list of options #32515
[x ] tests added / passed
[x ] passes black pandas
[x ] passes git diff upstream/master -u -- "*.py" | flake8 --diff
[x ] whatsnew entry

…_dict_orient

pandas/core/frame.py

elmonsomiat · 2020-03-07T13:48:34Z

doc/source/whatsnew/v1.1.0.rst

@@ -352,6 +352,7 @@ Other
  instead of ``TypeError: Can only append a Series if ignore_index=True or if the Series has a name`` (:issue:`30871`)
 - Set operations on an object-dtype :class:`Index` now always return object-dtype results (:issue:`31401`)
 - Bug in :meth:`AbstractHolidayCalendar.holidays` when no rules were defined (:issue:`31415`)
+- Using an `orient` argument not listed inside :meth:`DataFrame.to_dict` will raise a ``ValueError`` (:issue:`32515`)


@MomIsBestFriend I cannot see why the doc CI are failing with this added line. Not sure if this what's new info had to be added in some other place or I am not supposed to add this myself 🤔 Any ideas?
(btw, just checked and it works fine on my local machine)

@elmonsomiat I am not sure why the docs are failing, maybe try to merge master?

it looks like there are too many lines in this section (I tried adding my comment on another section within the same file and it works). Adding this new line breaks it. Not sure if it's OK to leave it as it is, without chaning the whatsnew file!

WillAyd · 2020-03-07T20:12:15Z

On board with the change but I think should wait until 2.0 since this could break things (though yes undocumented)

WillAyd · 2020-03-07T20:12:42Z

Should deprecate for 1.1

ShaharNaveh

Overall LGTM. ex @WillAyd comment that we should deprecate it, I believe that we should at least give a Deprecation warning. so I was thinking some thing with the style of:

orient = orient.lower()

if orient in {'i', 's', 'sp', ... }:
    warnings.warn("Using short name for 'orient' is deprecated...", DeprecationWarning, stacklevel=2)
    if orient == "i":
        orient = "index"
    elif orient == "l":
         orient = "list"
     ...

And then below the rest of your code.

… of orient

ShaharNaveh · 2020-03-08T21:51:37Z

I see that in c598b32 you reverted some code, pytest handles DeprecationWarning a bit different than the usual

with pytest.raises(Exception):
    foo()

You can read more about how pytest deals with DeprecationWarning at https://docs.pytest.org/en/latest/warnings.html#ensuring-code-triggers-a-deprecation-warning

To sum it up, you catch DeprecationWarning by:

with pytest.deprecated_call():
    foo()

ShaharNaveh

Besides the comments in #32516 (comment), this looks very good (IMO)

elmonsomiat · 2020-03-08T21:59:35Z

@MomIsBestFriend Thanks for the patient review! 👏

ShaharNaveh

This is the only comment I have, otherwise this is LGTM.

ShaharNaveh · 2020-03-09T12:22:28Z

pandas/tests/frame/methods/test_to_dict.py

@@ -64,14 +64,24 @@ def test_to_dict_index_not_unique_with_index_orient(self):
        with pytest.raises(ValueError, match=msg):
            df.to_dict(orient="index")

-    def test_to_dict_invalid_orient(self):
+    @pytest.mark.parametrize("orient", ["xinvalid"])


I think that parameteration is redundant, can you remove it and have this test as it was before

yeah, agreed

WillAyd

very nice! lgtm

ShaharNaveh

LGTM

jreback · 2020-03-11T03:39:35Z

doc/source/whatsnew/v1.1.0.rst

@@ -174,7 +174,7 @@ Deprecations
 - Lookups on a :class:`Series` with a single-item list containing a slice (e.g. ``ser[[slice(0, 4)]]``) are deprecated, will raise in a future version.  Either convert the list to tuple, or pass the slice directly instead (:issue:`31333`)
 - :meth:`DataFrame.mean` and :meth:`DataFrame.median` with ``numeric_only=None`` will include datetime64 and datetime64tz columns in a future version (:issue:`29941`)
 - Setting values with ``.loc`` using a positional slice is deprecated and will raise in a future version.  Use ``.loc`` with labels or ``.iloc`` with positions instead (:issue:`31840`)
-
+- :meth:`DataFrame.to_dict` will not accept short names for ``orient`` in future versions (:issue:`32515`)


has deprecated accepting short names

pandas/core/frame.py

…_dict_orient

…s into feature/to_dict_orient

pandas/core/frame.py

jreback · 2020-03-14T20:01:53Z

/azp run

azure-pipelines · 2020-03-14T20:02:03Z

Azure Pipelines successfully started running 1 pipeline(s).

WillAyd · 2020-03-14T21:33:19Z

Thanks @elmonsomiat - very impressive first contribution

…#32516)

shubh2u · 2020-05-31T22:55:54Z

Just reading through some PRs to understand the contribution process better. I see this sentence in the documentation "Abbreviations are allowed. s indicates series and sp indicates split." Is a corresponding doc change needed for this?

https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.to_dict.html

shubh2u · 2020-05-31T23:25:39Z

pandas/core/frame.py

@@ -1401,11 +1401,45 @@ def to_dict(self, orient="dict", into=dict):
            )
        # GH16122
        into_c = com.standardize_mapping(into)
-        if orient.lower().startswith("d"):
+
+        orient = orient.lower()


Line 1328 above might also need a change?

I would think so - are you interested in submitting a PR to update the documentation?

this needs to be removed in a future PR. Check below, there is a FutureWarning that says this will not be accepted anymore, then we can remove this line.

Cool, so to confirm, in the future any code like
df.to_dict(orient='s') will stop working?

yep, it should

elmonsomiat · 2020-06-01T10:33:15Z

Oh, yes, this sentence should be removed. This is not the case for to_json and I think they should be consistent.

elmonsomiat added 8 commits March 6, 2020 19:35

updated to dict function and tests involved

3cf3ed9

rolled back to python_env in yaml file

2e783e0

added space between lines

dcc4c2e

added space between lines

b755897

documented tests0

dc1207b

formated to black

1a78f3c

Merge branch 'master' of github.com:pandas-dev/pandas into feature/to…

0e5a874

…_dict_orient

added whats new

43d702c

ShaharNaveh reviewed Mar 7, 2020

View reviewed changes

pandas/core/frame.py Show resolved Hide resolved

elmonsomiat commented Mar 7, 2020

View reviewed changes

elmonsomiat added 4 commits March 7, 2020 14:18

removed line from doc whats new

e52a3b2

added comment on other enhancements to test the ci docs

ad5afa5

removed changes from docs

f161803

reset whatsnew file

a2edef1

added orient lower back

e069a60

elmonsomiat requested a review from ShaharNaveh March 8, 2020 12:03

ShaharNaveh suggested changes Mar 8, 2020

View reviewed changes

elmonsomiat added 6 commits March 8, 2020 20:59

added deprecation warning and rolled back to accepting short versions…

7075467

… of orient

updated black formatting

fb7de33

removed full orient strings to return warning"

4164a87

removed deprecation warning test

c598b32

added whatsnew for deprecation

7b9e90f

pulled master to get whatsnew changes

3d6efa5

added deprecation warning test

6e117ee

ShaharNaveh suggested changes Mar 8, 2020

View reviewed changes

added deprecation warning test

36f9561

updated test name

9e3daba

elmonsomiat requested review from mroeschke, WillAyd and ShaharNaveh March 9, 2020 10:12

ShaharNaveh suggested changes Mar 9, 2020

View reviewed changes

rolled back to original ValueError test for .to_dict

d1b9c50

WillAyd approved these changes Mar 10, 2020

View reviewed changes

ShaharNaveh approved these changes Mar 10, 2020

View reviewed changes

jreback requested changes Mar 11, 2020

View reviewed changes

jorisvandenbossche mentioned this pull request Mar 11, 2020

DEPR: log of deprecations in 1.x (to be removed in 2.0) #30228

Closed

updated whatsnew

9557349

elmonsomiat requested a review from jreback March 13, 2020 10:05

elmonsomiat mentioned this pull request Mar 13, 2020

df.to_dict() accepts orient which are not in the list of options #32515

Closed

jreback requested changes Mar 14, 2020

View reviewed changes

pandas/core/frame.py Outdated Show resolved Hide resolved

pandas/core/frame.py Show resolved Hide resolved

elmonsomiat added 3 commits March 14, 2020 17:06

moved mapping of strings inside if statements

dcc9d43

Merge branch 'master' of github.com:pandas-dev/pandas into feature/to…

228b71e

…_dict_orient

Merge branch 'feature/to_dict_orient' of github.com:elmonsomiat/panda…

beb1f32

…s into feature/to_dict_orient

elmonsomiat requested a review from jreback March 14, 2020 17:12

remvoed unused contains upper

4173ce6

jreback approved these changes Mar 14, 2020

View reviewed changes

pandas/core/frame.py Show resolved Hide resolved

jreback added this to the 1.1 milestone Mar 14, 2020

WillAyd merged commit e734449 into pandas-dev:master Mar 14, 2020

SeeminSyed pushed a commit to CSCD01-team01/pandas that referenced this pull request Mar 22, 2020

Deprecate Aliases as orient Argument in DataFrame.to_dict (pandas-dev…

6d2445f

…#32516)

shubh2u reviewed May 31, 2020

View reviewed changes

phofl mentioned this pull request Oct 24, 2022

DEP: Disallow abbreviations for orient in to_dict #49289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate Aliases as orient Argument in DataFrame.to_dict #32516

Deprecate Aliases as orient Argument in DataFrame.to_dict #32516

elmonsomiat commented Mar 7, 2020 •

edited

elmonsomiat Mar 7, 2020 •

edited

ShaharNaveh Mar 7, 2020

elmonsomiat Mar 7, 2020

WillAyd commented Mar 7, 2020

WillAyd commented Mar 7, 2020

ShaharNaveh left a comment

ShaharNaveh commented Mar 8, 2020

ShaharNaveh left a comment

elmonsomiat commented Mar 8, 2020

ShaharNaveh left a comment

ShaharNaveh Mar 9, 2020

elmonsomiat Mar 9, 2020

WillAyd left a comment

ShaharNaveh left a comment

jreback Mar 11, 2020

jreback commented Mar 14, 2020

azure-pipelines bot commented Mar 14, 2020

WillAyd commented Mar 14, 2020

shubh2u commented May 31, 2020

shubh2u May 31, 2020

MarcoGorelli Jun 1, 2020

elmonsomiat Jun 1, 2020

shubh2u Jun 1, 2020

elmonsomiat Jun 1, 2020

elmonsomiat commented Jun 1, 2020

Deprecate Aliases as orient Argument in DataFrame.to_dict #32516

Deprecate Aliases as orient Argument in DataFrame.to_dict #32516

Conversation

elmonsomiat commented Mar 7, 2020 • edited

elmonsomiat Mar 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Mar 7, 2020

WillAyd commented Mar 7, 2020

ShaharNaveh left a comment

Choose a reason for hiding this comment

ShaharNaveh commented Mar 8, 2020

ShaharNaveh left a comment

Choose a reason for hiding this comment

elmonsomiat commented Mar 8, 2020

ShaharNaveh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

ShaharNaveh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Mar 14, 2020

azure-pipelines bot commented Mar 14, 2020

WillAyd commented Mar 14, 2020

shubh2u commented May 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmonsomiat commented Jun 1, 2020

elmonsomiat commented Mar 7, 2020 •

edited

elmonsomiat Mar 7, 2020 •

edited