-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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: remove previously-deprecated get_value, set_value #27377
Conversation
@@ -1974,9 +1963,8 @@ def test_get_set_value_no_partial_indexing(self): | |||
# partial w/ MultiIndex raise exception | |||
index = MultiIndex.from_tuples([(0, 1), (0, 2), (1, 1), (1, 2)]) | |||
df = DataFrame(index=index, columns=range(4)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would blow most/all of the _get_value and _set_value test away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not sure we have other tests for _get_value/_set_value, which are still used internally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thought to migrating it to test_interals
then? I can see the value of separating these out from other tests at the very least
Note I have nothing against this PR itself, but I don't think we should be removing things after the RC. |
that makes sense, will move note to the 0.26.0 file |
(or, since there is no 0.26.0 file yet, will wait until there is) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm and indifferent on timing question
@@ -1974,9 +1963,8 @@ def test_get_set_value_no_partial_indexing(self): | |||
# partial w/ MultiIndex raise exception | |||
index = MultiIndex.from_tuples([(0, 1), (0, 2), (1, 1), (1, 2)]) | |||
df = DataFrame(index=index, columns=range(4)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thought to migrating it to test_interals
then? I can see the value of separating these out from other tests at the very least
Looks like these two methods are used in tests scattered across a handful of files. Getting a better organization for indexing tests is worthwhile, but is a pretty big task |
created a 0.26.0 file and moved the deprecation-removal there |
Are we just pretending like we're doing 0.26.0 for now, and moving to 1.0
if we decide we're ready? I think that makes the most sense.
…On Fri, Jul 19, 2019 at 10:02 AM jbrockmendel ***@***.***> wrote:
created a 0.26.0 file and moved the deprecation-removal there
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#27377?email_source=notifications&email_token=AAKAOIVJJRPM3O7EOVVZAYTQAHJPHA5CNFSM4ICW3Z3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2L4OFQ#issuecomment-513263382>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOIUQBC2VDK7XDRTZLTLQAHJPHANCNFSM4ICW3Z3A>
.
|
That's the impression I got from [dont remember, but I'm 80% sure it wasn't a dream] |
Just added the milestone - can change name as required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so are you sure we cannot simply remove entirely get_value and set_value? where exactly are these used outside of tests?
I see I think only
pandas/core//indexing.py: self.obj._set_value(*key, takeable=self._takeable)
which could be use iat/at
doc/source/whatsnew/v0.26.0.rst
Outdated
What's new in 0.26.0 (July XX, 2019) | ||
------------------------------------ | ||
|
||
Enhancements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added as 1.0, so this can be removed
doc/source/whatsnew/v0.26.0.rst
Outdated
|
||
Removal of prior version deprecations/changes | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
- Removed the previously deprecated :meth:`Series.get_value`, :meth:`Series.set_value`, :meth:`DataFrame.get_value`, :meth:`DataFrame.set_value` (:issue:`17739`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to 1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved
I count 15 non-test usages |
show me the list |
Searching 1589 files for "._get_value(|._set_value(" (regex, case sensitive) pandas/core/frame.py: pandas/core/indexing.py: pandas/core/sparse/frame.py: pandas/core/sparse/series.py: pandas/tests/frame/test_api.py: pandas/tests/frame/test_constructors.py: pandas/tests/frame/test_indexing.py: pandas/tests/series/indexing/test_datetime.py: pandas/tests/series/indexing/test_indexing.py: 37 matches across 9 files |
ok I guess this is not trivial to remove. However, this does need rationalization (e.g. the name is not inline with any other indexers, even internally that we have). So this should be addressed at some point; these routines likely are not great at handling for example non-basic dtypes as these never got love and have been externally replaced by iat/at. So if we don't have an issue can you create one.thanks. |
@jbrockmendel if you can't tell, I really really dislike doing these private version of public methods just so we can deprecate them, rather than actually fix things. This is just a propagation of technical debt and really hurting the project (just a bit of venting, you are certainly addressing lots of this prior techinical debt). |
ping on green. |
Definitely agreed on getting rid of the private versions before long. I've been looking at indexing methods and hoping to get some real simplification done before long. |
Ping |
Updated to also remove the deprecated methods from SparseSeries, SparseDataFrame. It looks like iat and at dispatch to _get_value/_set_value. After this I'd like to do a follow-up to move the contents of _get_value/_set_value to iat/at and get rid of _get_value/_set_value altogether. |
can you rebase again just to make sure here |
ping |
thanks |
…7377) * DEPR: remove previously-deprecated get_value, set_value * start 0.26.0 file, move deprecation removal there * whitespace fixup * move note to 1.0 file * also remove for Sparse * remove usages of get_value, set_value
Deprecated in 0.21.0