DEPR: Series ndarray properties (strides, data, base, itemsize, flags) by jorisvandenbossche · Pull Request #20721 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation23 Commits10 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- closes Deprecate Series.strides, .base, .data, .itemsize, .flags (numpy) attributes ? #20419
- tests added / passed
- passes
git diff upstream/master -u -- "*.py" | flake8 --diff - whatsnew entry
| @@ -737,11 +737,17 @@ def item(self): |
|---|
| @property |
| def data(self): |
| """ return the data pointer of the underlying data """ |
| warnings.warn("Series/Index.data is deprecated and will be " |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice to make the warning messages a bit more dynamic with something like:
"{obj}.data is deprecated...".format(obj=type(self).name)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you (somehow I was the docstring mindset thinking we needed to adapt this upon class definition, but of course this can be done dynamically on run time :-))
Hello @jorisvandenbossche! Thanks for updating the PR.
Cheers ! There are no PEP8 issues in this Pull Request. 🍻
Comment last updated on April 24, 2018 at 12:42 Hours UTC
| @@ -328,7 +328,7 @@ def test_series_agg_multi_pure_python(): |
|---|
| 'F': np.random.randn(11)}) |
| def bad(x): |
| assert (len(x.base) > 0) |
| assert (len(x.values.base) > 0) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really sure what the purpose of this assert actually is (was introduced in 71e9046)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls run all tests and see if any warnings comes up
| """ return the base object if the memory of the underlying data is |
|---|
| shared |
| """ |
| # override deprecated property in IndexOpsMixin, as we still need |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your explanation doesn't make sense, we are deprecating these no? you need to change the way its accessed in the code to remove the warnings.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explanation perfectly makes sense, but you don't like the way I solved it I suppose (I agree it is not the nicest, but I was thinking that this would only be temporarily until DatetimeTZBlock is an ExtensionBlock).
To fix the usage itself, .base is used in two placed:
Block.is_view: this I can override inDatetimeTzBlockto checkself.values.values.baseconcatenate_join_units:if copy and concat_values.base is not None: Not fully sure about this one what to do here.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my point is this method should not exist (as you are ddeprecateding), and would rather have you fix the usage
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said, I agree with that. Do you have a suggestion for the second case? (inside concatenate_join_units) In #20745 I need to touch the same code, and checked if the values have a base attribute, but that also feels hacky
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, see my comment there, only check base if its an ndarray
| @@ -1491,6 +1491,8 @@ def take_nd(arr, indexer, axis=0, out=None, fill_value=np.nan, mask_info=None, |
|---|
| if is_sparse(arr): |
| arr = arr.get_values() |
| arr = np.array(arr, copy=False) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np.asarray slightly more idiomatic
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes (for some reason I wanted the copy=False keyword, but for asarray that is of course just the default behaviour)
| """ return the base object if the memory of the underlying data is |
|---|
| shared |
| """ |
| # override deprecated property in IndexOpsMixin, as we still need |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my point is this method should not exist (as you are ddeprecateding), and would rather have you fix the usage
Now that #20745 is merged, updated this.
@jorisvandenbossche a couple new warnings I think: https://travis-ci.org/pandas-dev/pandas/jobs/369977719#L2762
pandas/tests/indexes/datetimes/test_datetimelike.py::TestDatetimeIndex::()::test_copy_name
/Users/travis/build/pandas-dev/pandas/pandas/core/indexes/base.py:551: FutureWarning: DatetimeIndex.base is deprecated and will be removed in a future version
orig = orig if orig.base is None else orig.base
pandas/tests/internals/test_internals.py::TestBlockManager::()::test_copy
/Users/travis/build/pandas-dev/pandas/pandas/tests/internals/test_internals.py:469: FutureWarning: DatetimeIndex.base is deprecated and will be removed in a future version
assert cp_blk.values.base is blk.values.base
/Users/travis/build/pandas-dev/pandas/pandas/tests/internals/test_internals.py:477: FutureWarning: DatetimeIndex.base is deprecated and will be removed in a future version
if cp_blk.values.base is not None and blk.values.base is not None:
/Users/travis/build/pandas-dev/pandas/pandas/tests/internals/test_internals.py:478: FutureWarning: DatetimeIndex.base is deprecated and will be removed in a future version
assert cp_blk.values.base is not blk.values.base
Can you see if they're easy to fix?
lgtm. modulo any warnings fixes. note that I just pushed a CI fix for lint (on other PR)
| @@ -548,6 +548,9 @@ def _deepcopy_if_needed(self, orig, copy=False): |
|---|
| """ |
| if copy: |
| # Retrieve the "base objects", i.e. the original memory allocations |
| if not isinstance(orig, np.ndarray): |
| # orig is a DatetimeIndex |
| orig = orig.values |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jreback This special case is because we sometimes construct DatetimeIndex objects where the _data attribute still is a DatetimeIndex in itself.
To me this seems like something we should not do? (it doesn't hurt, but there is also no reason to have it I think, so better always store the ndarray to be consistent?)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should never do that
how did this come up?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just opened an issue about it: #20810
It came up from fixing the warnings in the tests. The above case, where orig is not always a ndarray (hence my special case if statement), triggered it, because orig always comes from a _data attribute, and so I expected it to always be a ndarray.
I was doing scipy.fftpack.fft over a series and it showed error as
Attriute error: Series object has no attribute flags.
What should I use instead? And how to overcome it?