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

DEPR: Deprecate NDFrame.as_matrix #18458

Merged
merged 3 commits into from Nov 26, 2017

Conversation

topper-123
Copy link
Contributor

Deprecating NDFrame.as_matrix as per discussion in #18262.

@topper-123 topper-123 force-pushed the deprecate_as_matrix branch 2 times, most recently from d981cb8 to 7fd4b71 Compare November 24, 2017 00:34
@codecov
Copy link

codecov bot commented Nov 24, 2017

Codecov Report

Merging #18458 into master will increase coverage by 0.02%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18458      +/-   ##
==========================================
+ Coverage    91.3%   91.32%   +0.02%     
==========================================
  Files         163      163              
  Lines       49781    49783       +2     
==========================================
+ Hits        45451    45464      +13     
+ Misses       4330     4319      -11
Flag Coverage Δ
#multiple 89.12% <85.71%> (+0.02%) ⬆️
#single 40.72% <64.28%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/panel.py 96.85% <0%> (-0.29%) ⬇️
pandas/core/internals.py 94.47% <100%> (ø) ⬆️
pandas/core/generic.py 95.78% <83.33%> (+0.05%) ⬆️
pandas/plotting/_converter.py 65.25% <0%> (+1.81%) ⬆️

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 38f41e6...48e1fc8. Read the comment docs.

@jorisvandenbossche jorisvandenbossche added the Deprecate Functionality to remove in pandas label Nov 24, 2017
@jorisvandenbossche jorisvandenbossche added this to the 0.22.0 milestone Nov 24, 2017
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Left some comments

@@ -82,7 +82,7 @@ Deprecations
~~~~~~~~~~~~

- ``Series.from_array`` and ``SparseSeries.from_array`` are deprecated. Use the normal constructor ``Series(..)`` and ``SparseSeries(..)`` instead (:issue:`18213`).
-
- ``NDFrame.as_matrix`` is deprecated. Use ``NDFrame.values`` instead (:issue:`18458`).
Copy link
Member

Choose a reason for hiding this comment

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

Can you use DataFrame instead of NDFrame (basically NDFrame should never show up in the docs, it is an internal implementation detail, although there are some places where it leaks through ..)

@@ -3735,6 +3735,9 @@ def _get_bool_data(self):

def as_matrix(self, columns=None):
"""
DEPRECATED: This method will be removed in a future version.
Use :meth:`NDFrame.values` instead.
Copy link
Member

Choose a reason for hiding this comment

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

same comment here

@@ -3770,6 +3773,8 @@ def as_matrix(self, columns=None):
--------
pandas.DataFrame.values
"""
warnings.warn("This method will be removed in a future version. "
"Use ``.values`` instead.")
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be a FutureWarning, and you also need to set a stacklevel (this is a top-level method, stacklevel=2 should be the correct one)

Copy link
Member

Choose a reason for hiding this comment

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

Can you also mention as_matrix explicitly in the message (that is clearer as you don't always directly see what line or what method on a certain line is causing the warning).
And also follow a bit the typical message like "'as_matrix' is deprecated and will be removed in a future version. Use .."

@@ -243,9 +243,9 @@ def test_itertuples(self):
def test_len(self):
assert len(self.frame) == len(self.frame.index)

def test_as_matrix(self):
def test_values(self):
Copy link
Member

Choose a reason for hiding this comment

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

Can you leave one test (or now add a test_as_matrix_deprecated that copies eg the first case of this test) that uses as_matrix and that asserts it raises a warning and is the same as .values

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. I've added in below test_repr_with_mi_nat

@jorisvandenbossche
Copy link
Member

General question: I am wondering if we shouldn't rather recommend to do np.asarray(df) instead of df.values ?

@topper-123
Copy link
Contributor Author

Interesting, does it conform to .values in all cases?.

Anyway, I suggest that that will be a separate issue from this one.

@topper-123
Copy link
Contributor Author

I've changed the PR according to the comments. Is this ok?

@jreback jreback mentioned this pull request Nov 24, 2017
34 tasks
@@ -3791,7 +3796,10 @@ def values(self):
int32. By numpy.find_common_type convention, mixing int64 and uint64
will result in a flot64 dtype.
"""
return self.as_matrix()
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 .values

Copy link
Member

Choose a reason for hiding this comment

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

This is the def of .values

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, then we should push this entirely down to the BlockManager, so pass the axis as well. The BM can then do the transpose, we don't like to do things in user code like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should actually deprecate .values as well, in favor of .to_numpy() which is the future spelling anyhow.

Copy link
Member

Choose a reason for hiding this comment

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

This is already almost entirely pushed down, since it is calling directly BlockManager.as_matrix (but OK, the transpose could be done inside BlockManager.as_matrix). The consolidate_inplace needs to happens in NDFrame I think.

Deprecating .values is a no-go IMO, but let's deprecate that in another issue if you want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, then we should push this entirely down to the BlockManager, so pass the axis as well. The BM can then do the transpose, we don't like to do things in user code like this.

I'm not too familiar with the BlockManager, except accessing it with ._data. How do I pass the axis to it, and how do I transpose in a BlockManager? E.g. it doesn't have a .transpose method.

Copy link
Contributor

Choose a reason for hiding this comment

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

look at _take in pandas/core/generic.py, you just pass the get_block_manager_axis to it.

then add axis= to def as_matrix in pandas/core/internals and do the transpose there.

The reason for pushing this to internals is to avoid all of the wrapping layers (e.g. frame/series, etc.) to know about internal details like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as_matrix doesn't have a axis parameter. Do you mean adding a new axis parameter there and transposing inside, if axis=0 (where I assume from my limited knowledge that BlockManager.axes[0] is always the same as dataFrame.axes[1], correct?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jreback, I've tried quite a bit today and I can't push this down while getting the tests to pass.

Could you look at my latest commit (not passing ATM) and give some advice?

@@ -369,6 +369,12 @@ def test_values(self):
self.frame.values[:, 0] = 5.
assert (self.frame.values[:, 0] == 5).all()

def test_as_matrix_deprecated(self):
with tm.assert_produces_warning(FutureWarning):
Copy link
Contributor

Choose a reason for hiding this comment

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

add the issue number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

@topper-123 topper-123 force-pushed the deprecate_as_matrix branch 5 times, most recently from 61c5a2d to 579d128 Compare November 25, 2017 22:46
if len(self.blocks) == 0:
return np.empty(self.shape, dtype=float)

other_axis = abs(axis-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

don't do this

if items is not None:
mgr = self.reindex_axis(items, axis=0)
mgr = self.reindex_axis(items, axis=other_axis)
Copy link
Contributor

Choose a reason for hiding this comment

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

leave this alone

return self.as_matrix()
self._consolidate_inplace()
bm_axis = self._get_block_manager_axis(axis=1)
return self._data.as_matrix(axis=bm_axis)
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of this, just pass transpose = self._AXIS_REVERSED in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But _data.as_matrix has no transpose parameter, do you mean axis=self._AXIS_REVERSED?

Copy link
Contributor

Choose a reason for hiding this comment

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

no I mean add the argument transpose=False rather than axis prob simpler for now.

else:
mgr = self

if self._is_single_block or not self.is_mixed_type:
return mgr.blocks[0].get_values()
arr = mgr.blocks[0].get_values()
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

change this to transpose instead. should be straightforward from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot, transpose is of course much better than axis.

The issue was actually in the if len(self.blocks) == 0: block, as the empty array also must be transposed.

Everything is green now locally and I've pushed that upstream.

@topper-123
Copy link
Contributor Author

topper-123 commented Nov 26, 2017

A thought: BlockManager.as_matrix is a strange name as it implies a np.matrix is returned.

Should I not just change it to BlockManager.as_array? Or, do any external libraries rely on calling BlockManager directly, e.g. statsmodel?

@jreback
Copy link
Contributor

jreback commented Nov 26, 2017

@topper-123 ok this lgtm. ping on green.

@jreback
Copy link
Contributor

jreback commented Nov 26, 2017

Should I not just change it to BlockManager.as_array? Or, do any external libraries rely on calling BlockManager directly, e.g. statsmodel?

this is completely private and internal. as_array would be fine. while you are at it, can you add a doc-string.

ping when ready.

self._consolidate_inplace()
if self._AXIS_REVERSED:
return self._data.as_matrix(columns).T
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 just return self.values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning self.values implies that columns=None which isn't necessary true for user code.

@@ -3842,7 +3848,7 @@ def as_blocks(self, copy=True):
.. deprecated:: 0.21.0

NOTE: the dtypes of the blocks WILL BE PRESERVED HERE (unlike in
as_matrix)
as_array)
Copy link
Contributor

Choose a reason for hiding this comment

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

leave this spelling, this is a top-level method name

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.

@@ -3670,19 +3670,22 @@ def copy(self, deep=True, mgr=None):
return self.apply('copy', axes=new_axes, deep=deep,
do_integrity_check=False)

def as_matrix(self, items=None):
def as_array(self, transpose=False, items=None):
if len(self.blocks) == 0:
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 add a doc-string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@topper-123 topper-123 force-pushed the deprecate_as_matrix branch 2 times, most recently from 0816d86 to 6ad82de Compare November 26, 2017 00:33
@@ -3770,10 +3773,12 @@ def as_matrix(self, columns=None):
--------
pandas.DataFrame.values
"""
warnings.warn("method ``as_matrix`` will be removed in a future version. "
"Use ``values`` instead.", FutureWarning, stacklevel=2)
self._consolidate_inplace()
if self._AXIS_REVERSED:
Copy link
Contributor

Choose a reason for hiding this comment

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

ok so this should be the same as below (e.g. passing transpose=). make sure we still have a test on this (e.g. that passes columns).

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, and test_as_matrix_deprecated has been modified to a take columns param.

@topper-123 topper-123 force-pushed the deprecate_as_matrix branch 2 times, most recently from eb0b82d to e1066d2 Compare November 26, 2017 01:00
@@ -3770,10 +3773,11 @@ def as_matrix(self, columns=None):
--------
pandas.DataFrame.values
"""
warnings.warn("method ``as_matrix`` will be removed in a future version. "
Copy link
Contributor

Choose a reason for hiding this comment

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

"method method .as_matrix()...."
"Use .values instead"

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 not sure I understand, assume you want the backticks gone. Change uploaded.

@topper-123 topper-123 force-pushed the deprecate_as_matrix branch 5 times, most recently from d8b4d0b to a343fe3 Compare November 26, 2017 11:25
@topper-123
Copy link
Contributor Author

All green, @jreback

@jreback jreback merged commit f26bed6 into pandas-dev:master Nov 26, 2017
@jreback
Copy link
Contributor

jreback commented Nov 26, 2017

thanks @topper-123

@topper-123 topper-123 deleted the deprecate_as_matrix branch November 26, 2017 15:07
giantZorg added a commit to giantZorg/multivariate_time_series_forecasting that referenced this pull request Feb 4, 2020
as_matrix() got removed with Pandas 1.0.0 (pandas-dev/pandas#18458), replaced with values
atangfan added a commit to atangfan/Deep-Portfolio-Management that referenced this pull request May 11, 2020
See GH18458(pandas-dev/pandas#18458). Since as_matrix is deprecated for pd.DataFrame and pd.Series, use data.values instead.
jnirschl added a commit to jnirschl/risk-slim that referenced this pull request Aug 10, 2020
Pandas.DataFrame.as_matrix is deprecated since version 0.23.0 (see docs
and PR 18458). According to the documentation for Pandas 0.25.1, the
recommended function is DataFrame.to_numpy() in place of DataFrame.values
or DataFrame.as_matrix().

pandas-dev/pandas#18458
https://pandas.pydata.org/pandas-docs/version/0.23/generated/pandas.DataFrame.as_matrix.html
https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.values.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants