BUG: DataFrame.melt gives unexpected result when column "value" already exists by pizzathief · Pull Request #35003 · 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
Conversation14 Commits28 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 }})
- [ x] closes BUG: DataFrame.melt gives unexpected result when column "value" already exists #34731
- [x ] tests added / passed
- [ X] passes
black pandas - [ x] passes
git diff upstream/master -u -- "*.py" | flake8 --diff - [ x] whatsnew entry
Linting .py code
16
##[error]./pandas/core/apply.py:9:1:F401:'pandas._libs.reduction as libreduction' imported but unused
should I remove this line as part of this PR? "libreduction" isn't mentioned anywhere else in the file pandas/core/apply.py , but on the other hand, its out of scope
Issue34731 1 BUG: DataFrame.melt gives unexpected result when column "value" already exists
Can you merge master to fix CI issue?
| # a name in the dataframe already (default name is "value") |
|---|
| df = pd.DataFrame({"col": list("ABC"), "value": range(10, 16, 2)}) |
| with warnings.catch_warnings(record=True) as w: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use tm.assert_produces_warning here (e.g., as in L580 of this file)?
a use of a warning detection. Warning.
| @@ -40,6 +41,14 @@ def melt( |
|---|
| else: |
| cols = list(frame.columns) |
| if value_name in frame.columns: |
| warnings.warn( |
| 'The value column name "%s" conflicts with an existing ' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use f-string syntax? We don't use the Py2 syntax any more
| warnings.warn( |
|---|
| 'The value column name "%s" conflicts with an existing ' |
| "column in the dataframe." % (value_name), |
| DeprecationWarning, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to a FutureWarning?
| @@ -1105,6 +1105,7 @@ Reshaping |
|---|
| - Fixed bug in :func:`melt` where melting MultiIndex columns with ``col_level`` > 0 would raise a ``KeyError`` on ``id_vars`` (:issue:`34129`) |
| - Bug in :meth:`Series.where` with an empty Series and empty ``cond`` having non-bool dtype (:issue:`34592`) |
| - Fixed regression where :meth:`DataFrame.apply` would raise ``ValueError`` for elements whth ``S`` dtype (:issue:`34529`) |
| - Issue warning if value column name already exists when using :meth:`DataFrame.melt` (:issue:`34731`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this to the Deprecations section?
| if value_name in frame.columns: |
|---|
| warnings.warn( |
| 'The value column name "%s" conflicts with an existing ' |
| "column in the dataframe." % (value_name), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we want to give users insight as to what they need to do before the deprecated behavior gets changed - can you try to clarify the message a bit?
| @@ -816,6 +816,7 @@ Deprecations |
|---|
| - :meth:`util.testing.assert_almost_equal` now accepts both relative and absolute |
| precision through the ``rtol``, and ``atol`` parameters, thus deprecating the |
| ``check_less_precise`` parameter. (:issue:`13357`). |
| - Issue warning if value column name already exists when using :meth:`DataFrame.melt` (:issue:`34731`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you start with DataFrame.melt (like the other notes above), and also say that this is deprecated and removed in future version.
| @@ -40,6 +41,16 @@ def melt( |
|---|
| else: |
| cols = list(frame.columns) |
| if value_name in frame.columns: |
| warnings.warn( |
| "This dataframe has a column name that matches the value column " |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put value in single quotes as this is an argument that the user passed. (and this should be value_name)
| if value_name in frame.columns: |
|---|
| warnings.warn( |
| "This dataframe has a column name that matches the value column " |
| f'name of the resultant melted dataframe (That being "{value_name})". ' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resultant -> resuling DataFrame. you don't need the parenthesized expression.
| warnings.warn( |
|---|
| "This dataframe has a column name that matches the value column " |
| f'name of the resultant melted dataframe (That being "{value_name})". ' |
| "In the future this will raise an error, please set the value_name " |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value_name in single quotes
| df = pd.DataFrame({"col": list("ABC"), "value": range(10, 16, 2)}) |
|---|
| with tm.assert_produces_warning(FutureWarning): |
| dfm = df.melt(id_vars="value") # noqa F841 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you create an expected value and use assert_frame_equal
also instead of dfm, call this result.
@pizzathief want to make a PR to enforce this deprecation?
Labels
Functionality to remove in pandas
Concat, Merge/Join, Stack/Unstack, Explode