DEPR: attrs #52152
DEPR: attrs #52152
Conversation
There was a problem hiding this comment.
Looks fine (looks like the warning may need filtering out in a test which uses the legacy pickle with attrs)
@jorisvandenbossche just to make sure we're all on the same page - is it only __finalize__ and metadata you need in geopandas?
From a look at the repo, it looks like there's a test that .attrs, and a benchmark, but other than that it doesn't look directly used?
| if isinstance(left, (pd.DataFrame, pd.Series)): | ||
| left.attrs = {} | ||
| with tm.assert_produces_warning(FutureWarning, match=warn_msg): |
There was a problem hiding this comment.
can probably remove this if you're already filtering the warnings from the test?
There was a problem hiding this comment.
I am -1 on deprecating attrs without first having more discussion about this.
(and ideally in a dedicated issue about it, that brings up a rationale for why we would remove this and what the alternatives are)
@jorisvandenbossche just to make sure we're all on the same page - is it only finalize and metadata you need in geopandas?
As I tried to explain on the issue (#51280 (comment)), those are unrelated. GeoPandas is a subclass, where we indeed make use of our subclassing capabilities (involving _metadata and __finalize__).
But attrs is a public API feature for end users providing some metadata functionality (that actually can avoid that you have to make a subclass for some basic metadata).
So with my GeoPandas-maintainer hat on, I don't have anything to do with attrs (and I also personally never use it). But this is public functionality that we should just treat as such and discuss why we would deprecate/remove that.
| @@ -114,6 +114,7 @@ Deprecations | |||
| - Deprecated 'method', 'limit', and 'fill_axis' keywords in :meth:`DataFrame.align` and :meth:`Series.align`, explicitly call ``fillna`` on the alignment results instead (:issue:`51856`) | |||
| - Deprecated 'broadcast_axis' keyword in :meth:`Series.align` and :meth:`DataFrame.align`, upcast before calling ``align`` with ``left = DataFrame({col: left for col in right.columns}, index=right.index)`` (:issue:`51856`) | |||
| - Deprecated the 'axis' keyword in :meth:`.GroupBy.idxmax`, :meth:`.GroupBy.idxmin`, :meth:`.GroupBy.fillna`, :meth:`.GroupBy.take`, :meth:`.GroupBy.skew`, :meth:`.GroupBy.rank`, :meth:`.GroupBy.cumprod`, :meth:`.GroupBy.cumsum`, :meth:`.GroupBy.cummax`, :meth:`.GroupBy.cummin`, :meth:`.GroupBy.pct_change`, :meth:`GroupBy.diff`, :meth:`.GroupBy.shift`, and :meth:`DataFrameGroupBy.corrwith`; for ``axis=1`` operate on the underlying :class:`DataFrame` instead (:issue:`50405`, :issue:`51046`) | |||
| - Deprecated :meth:`DataFrame.attrs`, :meth:`Series.attrs`, to retain the old attribute propagation override ``__finalize__`` in a subclass (:issue:`51280`) | |||
There was a problem hiding this comment.
I am not sure that creating a subclass is the advice we want to give to users to retain the behaviour (generally users should be very wary about creating a custom subclass).
There was a problem hiding this comment.
That seems reasonable. Did you have something else in mind?
There was a problem hiding this comment.
Did you have something else in mind?
No, I am not aware of a good alternative. But so that's the part we should discuss before deprecating it.
|
OK thanks for clarifying let's make a separate issue about this and discuss then, it's controversial enough it may warrant a pdep |
MarcoGorelli
added
the
Needs Discussion
|
Mothballing, will revisit in a few months to see if the pro-attrs folks make progress on getting it fully working. |
jbrockmendel
added
the
Mothballed
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.xref #51280