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

Melting with not present column does not produce error #23575

Merged
merged 34 commits into from
Nov 21, 2018

Conversation

michaelsilverstein
Copy link
Contributor

@michaelsilverstein michaelsilverstein commented Nov 8, 2018

@pep8speaks
Copy link

pep8speaks commented Nov 8, 2018

Hello @michaelsilverstein! Thanks for updating the PR.

Comment last updated on November 15, 2018 at 22:12 Hours UTC

@codecov
Copy link

codecov bot commented Nov 8, 2018

Codecov Report

Merging #23575 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #23575      +/-   ##
==========================================
+ Coverage   92.25%   92.29%   +0.03%     
==========================================
  Files         161      161              
  Lines       51383    51500     +117     
==========================================
+ Hits        47404    47531     +127     
+ Misses       3979     3969      -10
Flag Coverage Δ
#multiple 90.68% <100%> (+0.04%) ⬆️
#single 42.31% <10%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/reshape/melt.py 97.54% <100%> (+0.21%) ⬆️
pandas/core/arrays/datetimelike.py 95.96% <0%> (-0.19%) ⬇️
pandas/io/formats/format.py 97.76% <0%> (-0.12%) ⬇️
pandas/core/indexes/timedeltas.py 89.18% <0%> (-0.08%) ⬇️
pandas/core/arrays/integer.py 95.47% <0%> (-0.02%) ⬇️
pandas/tseries/frequencies.py 97.06% <0%> (-0.02%) ⬇️
pandas/tseries/offsets.py 96.98% <0%> (-0.01%) ⬇️
pandas/core/arrays/period.py 98.44% <0%> (-0.01%) ⬇️
pandas/core/util/hashing.py 98.4% <0%> (ø) ⬆️
pandas/plotting/_core.py 83.63% <0%> (ø) ⬆️
... and 38 more

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 a23f901...0db8838. Read the comment docs.

Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need tests and a release note.

pandas/core/reshape/melt.py Outdated Show resolved Hide resolved
pandas/core/reshape/melt.py Outdated Show resolved Hide resolved
@datapythonista datapythonista added Reshaping Concat, Merge/Join, Stack/Unstack, Explode Error Reporting Incorrect or improved errors from pandas labels Nov 9, 2018
@jreback jreback changed the title check for columns in dataframe Melting with not present column does not produce error Nov 11, 2018
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls add some tests and a whatsnew entry

@michaelsilverstein
Copy link
Contributor Author

@jreback would you be able to point me to some documentation on how to properly document tests and a whatsnew entry?

@TomAugspurger
Copy link
Contributor

All the contributing docs are at http://pandas-docs.github.io/pandas-docs-travis/contributing.html

release notes: http://pandas-docs.github.io/pandas-docs-travis/contributing.html#documenting-your-code

doc/source/whatsnew/v0.24.0.txt Outdated Show resolved Hide resolved
pandas/core/reshape/melt.py Outdated Show resolved Hide resolved
pandas/tests/reshape/test_melt.py Outdated Show resolved Hide resolved
pandas/tests/reshape/test_melt.py Outdated Show resolved Hide resolved
pandas/tests/reshape/test_melt.py Outdated Show resolved Hide resolved
pandas/tests/reshape/test_melt.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@michaelsilverstein michaelsilverstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jreback I have added tests and a whatsnew entry

doc/source/whatsnew/v0.24.0.txt Outdated Show resolved Hide resolved
pandas/core/reshape/melt.py Outdated Show resolved Hide resolved
pandas/tests/reshape/test_melt.py Outdated Show resolved Hide resolved
pandas/tests/reshape/test_melt.py Outdated Show resolved Hide resolved
pandas/tests/reshape/test_melt.py Outdated Show resolved Hide resolved
@TomAugspurger
Copy link
Contributor

TomAugspurger commented Nov 14, 2018 via email

# Conflicts:
#	doc/source/whatsnew/v0.24.0.rst
…v_melt_column_check

# Conflicts:
#	doc/source/whatsnew/v0.24.0.rst
…v_melt_column_check

# Conflicts:
#	doc/source/whatsnew/v0.24.0.rst
@michaelsilverstein
Copy link
Contributor Author

@jreback CircleCI and Azure have passed. Failure in TravisCI seems unrelated (@TomAugspurger agrees it seems).

@TomAugspurger
Copy link
Contributor

This failure is real: https://travis-ci.org/pandas-dev/pandas/jobs/455623394#L2875

It seems there's an issue with MultiIndex in the columns. I'm surprised there aren't tests for that outside of the docstring.

@michaelsilverstein
Copy link
Contributor Author

I'll add the tests from the docstring explicitly

@michaelsilverstein
Copy link
Contributor Author

I think this is the only error from TravisCI now?
https://travis-ci.org/pandas-dev/pandas/jobs/455723256#L2807

@jreback
Copy link
Contributor

jreback commented Nov 16, 2018

yeah this looks like a ci/code_checks.sh error, run it locally and see

@TomAugspurger
Copy link
Contributor

@michaelsilverstein
Copy link
Contributor Author

What is the proper order of imports? I had to import Index

@TomAugspurger
Copy link
Contributor

http://pandas-docs.github.io/pandas-docs-travis/contributing.html#import-formatting

SO something like isort pandas/core/reshape/melt.py

@michaelsilverstein
Copy link
Contributor Author

I think the failure from TravisCI passed here:
https://travis-ci.org/michaelsilverstein/pandas/builds/456028319

@@ -24,6 +25,10 @@
def melt(frame, id_vars=None, value_vars=None, var_name=None,
value_name='value', col_level=None):
# TODO: what about the existing index?
if isinstance(frame.columns, ABCMultiIndex):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not especially familiar with melt and multi-index columns, but I don't think this is quite right.

It seems like you need to specify col_level when you have a MI in the columns, so you should probably just be checks against frame.columns.levels[col_level] when you have a MI.

However, it doesn't quite seem that a col_level is required when there's a MI in the columns. The default of pd.melt(df) seems to work, but any time I specified an id_vars or value_vars without col_level I get an uninformative error message. I'm not sure what's going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to provide col_level for MI when only melting on one level like this example from the docstring (that I added a new test for):

pd.melt(df, col_level=0, id_vars=['A'], value_vars=['B'])

But you don't need to specify col_level when using all levels of MI:
pd.melt(df, id_vars=[('A', 'D')], value_vars=[('B', 'E')])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All I am doing at L28 is gathering column names from all levels. There are other checks to make sure that melting is performed properly, this will just check to make sure that whatever you pass, it is in your df

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i think this is ok, can you provdie a comment on what is going on.

doc/source/whatsnew/v0.24.0.rst Outdated Show resolved Hide resolved
@@ -24,6 +25,10 @@
def melt(frame, id_vars=None, value_vars=None, var_name=None,
value_name='value', col_level=None):
# TODO: what about the existing index?
if isinstance(frame.columns, ABCMultiIndex):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i think this is ok, can you provdie a comment on what is going on.

KeyError,
match=msg.format(Var='id_vars',
Col="\\['not_here', 'or_there'\\]")):
df.melt(['a', 'b', 'not_here', 'or_there'], ['c', 'd'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you do an example with an MI and columns that are not in the top level of the MI, ideally try with and w/o col_level as well.

@jreback
Copy link
Contributor

jreback commented Nov 21, 2018

lgtm. ping on green.

@jreback jreback merged commit 3e01c38 into pandas-dev:master Nov 21, 2018
@jreback
Copy link
Contributor

jreback commented Nov 21, 2018

thanks!

@michaelsilverstein michaelsilverstein deleted the dev_melt_column_check branch November 21, 2018 17:54
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Reporting Incorrect or improved errors from pandas Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Melting with not present column does not produce error
5 participants