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

Projects
None yet
5 participants
@michaelsilverstein
Copy link
Contributor

commented Nov 8, 2018

  • closes #23570
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
@pep8speaks

This comment has been minimized.

Copy link

commented Nov 8, 2018

Hello @michaelsilverstein! Thanks for updating the PR.

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

This comment has been minimized.

Copy link

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.

@TomAugspurger
Copy link
Contributor

left a comment

This will need tests and a release note.

Show resolved Hide resolved pandas/core/reshape/melt.py Outdated
Show resolved Hide resolved pandas/core/reshape/melt.py Outdated

@jreback jreback changed the title check for columns in dataframe Melting with not present column does not produce error Nov 11, 2018

@jreback
Copy link
Contributor

left a comment

pls add some tests and a whatsnew entry

@michaelsilverstein

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

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

@TomAugspurger

This comment has been minimized.

michaelsilverstein added some commits Nov 13, 2018

Show resolved Hide resolved 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
@michaelsilverstein
Copy link
Contributor Author

left a comment

@jreback I have added tests and a whatsnew entry

Show resolved Hide resolved 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
@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

michaelsilverstein added some commits Nov 15, 2018

Merge branch 'master' of https://github.com/pandas-dev/pandas into de…
…v_melt_column_check

# Conflicts:
#	doc/source/whatsnew/v0.24.0.rst
Merge branch 'master' of https://github.com/pandas-dev/pandas into de…
…v_melt_column_check

# Conflicts:
#	doc/source/whatsnew/v0.24.0.rst
@michaelsilverstein

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2018

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

@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2018

I'll add the tests from the docstring explicitly

@michaelsilverstein

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

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

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

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

@michaelsilverstein

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

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

@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

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

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

@michaelsilverstein

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

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):

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Nov 16, 2018

Contributor

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.

This comment has been minimized.

Copy link
@michaelsilverstein

michaelsilverstein Nov 16, 2018

Author Contributor

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')])

This comment has been minimized.

Copy link
@michaelsilverstein

michaelsilverstein Nov 16, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@michaelsilverstein

michaelsilverstein Nov 19, 2018

Author Contributor

Thoughts?

This comment has been minimized.

Copy link
@jreback

jreback Nov 19, 2018

Contributor

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

Show resolved Hide resolved doc/source/whatsnew/v0.24.0.rst Outdated
@@ -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):

This comment has been minimized.

Copy link
@jreback

jreback Nov 19, 2018

Contributor

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'])

This comment has been minimized.

Copy link
@jreback

jreback Nov 19, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

lgtm. ping on green.

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

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas Build #20181121.51 succeeded
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

thanks!

@michaelsilverstein michaelsilverstein deleted the michaelsilverstein:dev_melt_column_check branch Nov 21, 2018

Pingviinituutti added a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Pingviinituutti added 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
You can’t perform that action at this time.