DEPR: Deprecate convert_float by ahawryluk · Pull Request #41176 · 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
Conversation23 Commits13 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 }})
- closes CLN: consider deprecating convert_floats from read_excel #41127
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
I added tm.assert_produces_warning for every existing test that uses the convert_float argument, and a git grep only found one reference to convert_float in doc/. Is anything else required for a deprecation? IIUC, the actual feature would get removed in a pandas 2.0, correct?
Hello @ahawryluk! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻
Comment last updated at 2021-05-25 16:38:24 UTC
I'm failing some checks due the warnings stacklevel. I think stacklevel=5 would be correct if the user calls pd.read_excel, but then it would be wrong for pd.ExcelFile.parse. What would you recommend?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, pending the stacklevel issue. I think this can be handled similarly to how the deprecation of xlrd was done.
| caller = inspect.stack()[1] |
|---|
| if ( |
| caller.filename.endswith( |
| os.path.join("pandas", "io", "excel", "_base.py") |
| ) |
| and caller.function == "read_excel" |
| ): |
| stacklevel = 4 |
| else: |
| stacklevel = 2 |
| warnings.warn( |
| f"Your version of xlrd is {xlrd_version}. In xlrd >= 2.0, " |
| f"only the xls format is supported. Install " |
| f"openpyxl instead.", |
| FutureWarning, |
| stacklevel=stacklevel, |
IIUC, the actual feature would get removed in a pandas 2.0, correct?
Correct.
Thanks to rhshadrach for this trick
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| mangle_dupe_cols=True, |
|---|
| **kwds, |
| ): |
| if convert_float is None: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is slightly magical. can we instead have a function that shows the deprecation and just call it where appropriate so can set the stacklevel w/o this?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this would get around needing logic to set the stacklevel. Maybe you're asking to move the logic to the new function?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jreback @rhshadrach I've consolidated both stack inspections from _base.py into a single function. It seems like an improvement to me, but let me know if you see other adjustments I should make.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After rereading @jreback's comment, it now makes sense to me. The request is to move the deprecation itself to a function (not the determination of stacklevel), and call this further up in the call stack where we don't need to have any logic to determine the stacklevel. If this is doable, I agree it's a better approach.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would work: move the warnings.warn to a function, call in both read_excel and ExcelFile.parse when appropriate, but in read_excel after emitting the warning change the value of convert_float. Does that make sense?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might be able to use this: from pandas.util._exceptions import find_stack_level
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice. I looked at pandas.util._exceptions.find_stack_level when I made my last edit, but at the time it seemed specific to "astype" et al. so I tried making something similar. The edits from #41560 two days ago look great. I'll give it a try right away. (I couldn't see a clean way without stack inspection to raise a single warning from both read_excel and ExcelFile.parse. The latter isn't really advertised in the documentation, but it should still raise the correct warnings.)
This one is named after pandas.util._exceptions.find_stack_level
CLN: Deprecate convert_float DEPR: Deprecate convert_float
@ahawryluk can you merge master to resolve conflicts
That routine was recently generalized and works great
@jreback @rhshadrach I've updated the warnings in pandas/io/excel/_base.py to use the recently improved find_stack_level from pandas.utils._exceptions. One of the test suites failed, but I don't think it's related to excel routines. One of the error messages was "TypeError: Cannot cast array data from dtype('O') to dtype('<U') according to the rule 'unsafe'", and all the others were "FutureWarning: Promotion of numbers and bools to strings is deprecated."
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice - lgtm
Thanks everyone for your help
TLouf pushed a commit to TLouf/pandas that referenced this pull request
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request
