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

CLN: Remove Series._from_array #19893

Merged
merged 9 commits into from
Feb 27, 2018
Merged

Conversation

jaumebonet
Copy link
Contributor

@jaumebonet jaumebonet commented Feb 25, 2018

This PR transfers the class check of Series._from_array to DataFrame._box_col_values; removing the need for calling the Series._from_array and allowing for user-defined slice-inheritance of metadata.

I opted to delete the function as it was private but leave the Series.from_array public access working and it's FutureWarning of deprecation, as it was. To do that, I had to modify Series.from_array so it would keep also its original functionality until is completely deleted.

Transfer its check to DataFrame._box_col_values
@codecov
Copy link

codecov bot commented Feb 25, 2018

Codecov Report

Merging #19893 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19893      +/-   ##
==========================================
+ Coverage   91.65%   91.66%   +<.01%     
==========================================
  Files         150      150              
  Lines       48915    48939      +24     
==========================================
+ Hits        44835    44859      +24     
  Misses       4080     4080
Flag Coverage Δ
#multiple 90.04% <100%> (ø) ⬆️
#single 41.81% <90%> (-0.02%) ⬇️
Impacted Files Coverage Δ
pandas/core/sparse/series.py 95.2% <ø> (-0.05%) ⬇️
pandas/core/dtypes/concat.py 99.16% <100%> (+0.01%) ⬆️
pandas/core/series.py 94.2% <100%> (-0.21%) ⬇️
pandas/core/frame.py 97.18% <100%> (-0.05%) ⬇️
pandas/core/dtypes/cast.py 87.68% <0%> (-0.3%) ⬇️
pandas/core/generic.py 95.88% <0%> (-0.05%) ⬇️
pandas/core/indexes/datetimelike.py 97.38% <0%> (+0.01%) ⬆️
pandas/core/indexes/timedeltas.py 90.99% <0%> (+0.05%) ⬆️
pandas/core/groupby.py 92.31% <0%> (+0.1%) ⬆️
... and 2 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 10cc8f4...e4ec987. Read the comment docs.

@pep8speaks
Copy link

pep8speaks commented Feb 25, 2018

Hello @jaumebonet! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 26, 2018 at 14:55 Hours UTC

@@ -570,3 +570,56 @@ def strech(row):
result = df.apply(lambda x: [1, 2, 3], axis=1)
assert not isinstance(result, tm.SubclassedDataFrame)
tm.assert_series_equal(result, expected)

def test_frame_subclassing_and_inherit(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of these tests? this is another issue, yes?

Copy link
Contributor

Choose a reason for hiding this comment

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

this should be in a new PR

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 wrote it to check that that approach would work now. I'll take it out.

# This check here was previously performed in Series._from_array
# By doing it here there is no need for that function anymore
# GH#19883.
from pandas.core.dtypes.generic import ABCSparseArray
Copy link
Contributor

Choose a reason for hiding this comment

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

make this a function like _get_series_result_type and put it near _get_frame_result_type in pandas/core/dtypes/concat.py, it should just take self and return the type.

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 see the idea.
Wouldn't it make more sense to be in pandas/core/dtypes/commons.py ? As that file is already imported to frame.py?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, common is for introspection, not conversion

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!

@@ -2563,8 +2562,16 @@ def _box_item_values(self, key, values):

def _box_col_values(self, values, items):
""" provide boxed values for a column """
return self._constructor_sliced._from_array(values, index=self.index,
name=items, fastpath=True)
# This check here was previously performed in Series._from_array
Copy link
Contributor

Choose a reason for hiding this comment

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

remove all of this commentary

@jreback jreback added Sparse Sparse Data Type Clean labels Feb 25, 2018
to check type of DataFrame._constructor_sliced
@@ -101,6 +101,18 @@ def _get_frame_result_type(result, objs):
ABCSparseDataFrame))


def _get_sliced_frame_result_type(data, obj):
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

add a full doc-string

if is_sparse(data):
from pandas.core.sparse.api import SparseSeries
return SparseSeries
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

just return, no else needed

the data is sparse or not.
"""
if is_sparse(data):
from pandas.core.sparse.api import SparseSeries
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

@@ -2563,8 +2562,10 @@ def _box_item_values(self, key, values):

def _box_col_values(self, values, items):
""" provide boxed values for a column """
return self._constructor_sliced._from_array(values, index=self.index,
name=items, fastpath=True)
from pandas.core.dtypes.concat import _get_sliced_frame_result_type
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be imported at the top

return self._constructor_sliced._from_array(values, index=self.index,
name=items, fastpath=True)
from pandas.core.dtypes.concat import _get_sliced_frame_result_type
this_constructor_sliced = _get_sliced_frame_result_type(values, self)
Copy link
Contributor

Choose a reason for hiding this comment

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

call this klass

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.

small changes, ping on green


Parameters
----------
data : ndarray
Copy link
Contributor

Choose a reason for hiding this comment

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

array-like

Series or SparseSeries
"""
if is_sparse(data):
from pandas.core.sparse.api import SparseSeries
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 SparseSeries

@jaumebonet
Copy link
Contributor Author

done!

@jreback jreback added this to the 0.23.0 milestone Feb 26, 2018
@jreback
Copy link
Contributor

jreback commented Feb 26, 2018

thanks ping on green.

@jaumebonet
Copy link
Contributor Author

sorry... I'm not sure what "ping on green" means in this context 😅

@jreback
Copy link
Contributor

jreback commented Feb 26, 2018

it means watch this page and report when all of the checks are green.

@jreback jreback merged commit ceb9031 into pandas-dev:master Feb 27, 2018
@jreback
Copy link
Contributor

jreback commented Feb 27, 2018

thanks @jaumebonet

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Sparse Sparse Data Type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate Series._from_array ?
3 participants