Skip to content
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

DEPR: Deprecate tupleize_cols in read_csv #17820

Merged

Conversation

@gfyoung
Copy link
Member

commented Oct 8, 2017

@gfyoung gfyoung added this to the 0.21.0 milestone Oct 8, 2017

@gfyoung gfyoung force-pushed the forking-repos:tupleize-cols-read-csv-depr branch from 6ba8d46 to ee4e218 Oct 9, 2017

@codecov

This comment has been minimized.

Copy link

commented Oct 9, 2017

Codecov Report

Merging #17820 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17820      +/-   ##
==========================================
- Coverage   91.26%   91.24%   -0.02%     
==========================================
  Files         163      163              
  Lines       49980    49982       +2     
==========================================
- Hits        45613    45606       -7     
- Misses       4367     4376       +9
Flag Coverage Δ
#multiple 89.05% <100%> (ø) ⬆️
#single 40.25% <100%> (-0.06%) ⬇️
Impacted Files Coverage Δ
pandas/io/parsers.py 95.5% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.74% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f0ee53...ee4e218. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Oct 9, 2017

Codecov Report

Merging #17820 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17820      +/-   ##
==========================================
- Coverage   91.24%   91.22%   -0.02%     
==========================================
  Files         163      163              
  Lines       49992    49994       +2     
==========================================
- Hits        45613    45606       -7     
- Misses       4379     4388       +9
Flag Coverage Δ
#multiple 89.02% <100%> (ø) ⬆️
#single 40.24% <100%> (-0.06%) ⬇️
Impacted Files Coverage Δ
pandas/io/parsers.py 95.5% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.74% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ebe085...bee9012. Read the comment docs.

@gfyoung gfyoung force-pushed the forking-repos:tupleize-cols-read-csv-depr branch from ee4e218 to 75eb513 Oct 9, 2017

@@ -716,6 +716,7 @@ Deprecations

- :func:`read_excel()` has deprecated ``sheetname`` in favor of ``sheet_name`` for consistency with ``.to_excel()`` (:issue:`10559`).
- :func:`read_excel()` has deprecated ``parse_cols`` in favor of ``usecols`` for consistency with :func:`read_csv` (:issue:`4988`)
- :func:`read_csv()` has deprecated the ``tupleize_cols`` argument (:issue:`17060`)

This comment has been minimized.

Copy link
@jreback

jreback Oct 9, 2017

Contributor

you can say that this will always convert to a MI

This comment has been minimized.

Copy link
@gfyoung

gfyoung Oct 9, 2017

Author Member

Yep, done.

@jsexauer jsexauer referenced this pull request Oct 9, 2017

Open

DEPR: deprecations from prior versions #6581

0 of 89 tasks complete
@jreback

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2017

minor comment. merge when ready.

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Oct 9, 2017

@gfyoung The warning does not seem to appear when doing read_csv(..., tupleize_cols=False), so I think we need to put the default to None, so we can warn in those cases as well?

@gfyoung

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2017

@jorisvandenbossche : That's deliberate. We only issue a warning on non-default arguments. Also, tupleize_cols=False in the context read_csv means return an MI, so we're perfectly fine.

@gfyoung gfyoung force-pushed the forking-repos:tupleize-cols-read-csv-depr branch from 75eb513 to bee9012 Oct 9, 2017

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Oct 9, 2017

Why do we do that deliberately?
Nothing prevents a user to specify a default value, and eg to write pd.read_csv(..., tupleize_cols=False). This code will then break in the future without any warning.

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Oct 9, 2017

We only issue a warning on default arguments.

You mean 'non-default' arguments?

@gfyoung

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2017

You mean 'non-default' arguments?

Oops, yes 😄

Why do we do that deliberately?
Nothing prevents a user to specify a default value, and eg to write pd.read_csv(..., tupleize_cols=False).

That's possible. My concern is that's going to be confusing to a user if they see tupleize_cols=None in the docs. This deprecation methodology that I'm using is implemented throughout the codebase.

That being said, you do bring up an interesting point. While I haven't seen that problem arise with any of our other deprecations, I don't see why we shouldn't discuss it.

@gfyoung gfyoung merged commit 35590c6 into pandas-dev:master Oct 9, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gfyoung

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2017

@jorisvandenbossche : I opened #17828 to address your point about function argument deprecations.

@gfyoung gfyoung deleted the forking-repos:tupleize-cols-read-csv-depr branch Oct 9, 2017

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Oct 9, 2017

I disagree with not raising for the default value is how it is typically done. I think it is rather the other way around (we normally put the deprecated one at None, to be able to distinguish the default and an actual user passed value), but will answer on the issue you created.

ghost pushed a commit to reef-technologies/pandas that referenced this pull request Oct 16, 2017

alanbato added a commit to alanbato/pandas that referenced this pull request Nov 10, 2017

No-Stream added a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

@jreback jreback referenced this pull request Jun 29, 2019

Open

DEPR: deprecations log for removed issues #13777

141 of 141 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.