Skip to content

melt moved into its own module #18148

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

Merged
merged 3 commits into from
Nov 12, 2017
Merged

melt moved into its own module #18148

merged 3 commits into from
Nov 12, 2017

Conversation

tdpetrou
Copy link
Contributor

@tdpetrou tdpetrou commented Nov 7, 2017

This pull request is to make PR #17677 easier and was requested by @jreback here

@tdpetrou tdpetrou mentioned this pull request Nov 7, 2017
2 tasks
from pandas.core.dtypes.missing import notna
import pandas.core.dtypes.concat as _concat

from pandas.core.series import Series
Copy link
Contributor

Choose a reason for hiding this comment

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

just import from pandas as much as you can rather than the implementation (e.g. from pandas import DataFrame)

other='DataFrame.melt'))
def melt(frame, id_vars=None, value_vars=None, var_name=None,
value_name='value', col_level=None):
# TODO: what about the existing index?
Copy link
Contributor

Choose a reason for hiding this comment

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

this is all cut/paste?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I just cut and pasted everything. I thought that's what you wanted. Let me know if you wanted something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes just confirming

@@ -33,6 +33,7 @@
from pandas.core.frame import _shared_docs
from pandas.util._decorators import Appender
from pandas.core.index import Index, MultiIndex, _get_na_value
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect linting will fail here, you need to remove imports that are no longer needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I didn't modify anything. I wanted to make sure we were on the right track first.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh ok (cleanup imports here is fine though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok cleaned up imports

@jreback jreback added Clean Internals Related to non-user accessible pandas implementation labels Nov 7, 2017
from pandas.core.dtypes.common import is_list_like
from pandas import compat

from pandas.core.frame import DataFrame
Copy link
Contributor

Choose a reason for hiding this comment

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

import these from pandas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reshape.py uses the full path. Also, I'm getting an import error when I do from pandas import DataFrame

from pandas.core.index import Index, MultiIndex, _get_na_value
from pandas.core.reshape.melt import melt
Copy link
Contributor

Choose a reason for hiding this comment

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

this import should not be necessary, instead directly import from the new module

@jreback jreback added this to the 0.22.0 milestone Nov 8, 2017
@jreback
Copy link
Contributor

jreback commented Nov 8, 2017

rebase

i would also move lreshape and wide_to_long to melt.py

@@ -24,8 +24,8 @@
from pandas.core.panel import Panel, WidePanel
from pandas.core.panel4d import Panel4D
from pandas.core.reshape.reshape import (
Copy link
Contributor

Choose a reason for hiding this comment

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

see if you can move these reshape imports here to pandas.core.reshape.api (with the other ones); reshape.api is already imported to pandas.__init__, just make more sense there

from pandas.core.categorical import Categorical

from pandas.core.frame import DataFrame
from pandas.core.index import MultiIndex
Copy link
Contributor

Choose a reason for hiding this comment

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

you can do this now, or can in a followup. These top-level imports needs to be from the pandas namespace; some can be ABC, e.g. ABCMultiIndex, which you can import here. The others should be imported inside the functions themselves if needed. This allows this module to be imported irrespective of its import ordering.

if id_vars is not None:
if not is_list_like(id_vars):
id_vars = [id_vars]
elif (isinstance(frame.columns, MultiIndex) and
Copy link
Contributor

Choose a reason for hiding this comment

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

ABCMultiIndex

if value_vars is not None:
if not is_list_like(value_vars):
value_vars = [value_vars]
elif (isinstance(frame.columns, MultiIndex) and
Copy link
Contributor

Choose a reason for hiding this comment

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

same

frame.columns = frame.columns.get_level_values(col_level)

if var_name is None:
if isinstance(frame.columns, MultiIndex):
Copy link
Contributor

Choose a reason for hiding this comment

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

same

mdata[col] = np.asanyarray(frame.columns
._get_level_values(i)).repeat(N)

return DataFrame(mdata, columns=mcolumns)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can

from pandas import DataFrame here

mdata = {}
pivot_cols = []

for target, names in zip(keys, values):
Copy link
Contributor

Choose a reason for hiding this comment

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

you can
from pandas.core.dtypes.concat import _concat here

if not mask.all():
mdata = dict((k, v[mask]) for k, v in compat.iteritems(mdata))

return DataFrame(mdata, columns=id_cols + pivot_cols)
Copy link
Contributor

Choose a reason for hiding this comment

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

from pandas import DataFrame

@@ -11,8 +11,8 @@

from pandas.util.testing import assert_frame_equal

from pandas.core.reshape.reshape import (
melt, lreshape, get_dummies, wide_to_long)
from pandas.core.reshape.reshape import get_dummies
Copy link
Contributor

Choose a reason for hiding this comment

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

use from pandas import get_dummies, melt, lreshape, wide_to_long.

in tests we always want to use the user call (rather than a specific import)

@codecov
Copy link

codecov bot commented Nov 11, 2017

Codecov Report

Merging #18148 into master will decrease coverage by 0.01%.
The diff coverage is 96.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18148      +/-   ##
==========================================
- Coverage   91.42%   91.41%   -0.02%     
==========================================
  Files         163      164       +1     
  Lines       50068    50076       +8     
==========================================
- Hits        45777    45776       -1     
- Misses       4291     4300       +9
Flag Coverage Δ
#multiple 89.22% <96.29%> (ø) ⬆️
#single 40.36% <15.74%> (-0.06%) ⬇️
Impacted Files Coverage Δ
pandas/core/reshape/reshape.py 100% <ø> (+0.72%) ⬆️
pandas/core/reshape/api.py 100% <100%> (ø) ⬆️
pandas/core/frame.py 97.8% <100%> (-0.1%) ⬇️
pandas/core/api.py 100% <100%> (ø) ⬆️
pandas/core/reshape/melt.py 96.19% <96.19%> (ø)
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️

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 d8129b4...f5860a1. Read the comment docs.

@jorisvandenbossche
Copy link
Member

I would keep the code changes / clean-up for a follow-up PR, and keep it here to just moving things

@jreback jreback merged commit 9dc9d80 into pandas-dev:master Nov 12, 2017
@jreback
Copy link
Contributor

jreback commented Nov 12, 2017

thanks @tdpetrou

ok can you do a followup-PR to implement the small cleans before we move on to your proposed changes. thanks.

@tdpetrou tdpetrou mentioned this pull request Nov 13, 2017
@tdpetrou tdpetrou deleted the melt-only branch November 23, 2017 16:56
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Internals Related to non-user accessible pandas implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants