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

Various methods don't call call __finalize__ #28283

Open
34 of 38 tasks
TomAugspurger opened this issue Sep 4, 2019 · 48 comments
Open
34 of 38 tasks

Various methods don't call call __finalize__ #28283

TomAugspurger opened this issue Sep 4, 2019 · 48 comments
Assignees
Labels

Comments

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Sep 4, 2019

Improve coverage of NDFrame.__finalize__

Pandas uses NDFrame.__finalize__ to propagate metadata from one NDFrame to
another. This ensures that things like self.attrs and self.flags are not
lost. In general we would like that any operation that accepts one or more
NDFrames and returns an NDFrame should propagate metadata by calling
__finalize__.

The test file at
https://github.com/pandas-dev/pandas/blob/master/pandas/tests/generic/test_finalize.py
attempts to be an exhaustive suite of tests for all these cases. However there
are many tests currently xfailing, and there are likely many APIs not covered.

This is a meta-issue to improve the use of __finalize__. Here's a hopefully
accurate list of methods that don't currently call finalize.

Some general comments around finalize

  1. We don't have a good sense for what should happen to attrs when there are
    multiple NDFrames involved with differing attrs (e.g. in concat). The safest
    approach is to probably drop the attrs when they don't match, but this will
    need some thought.
  2. We need to be mindful of performance. __finalize__ can be somewhat expensive
    so we'd like to call it exactly once per user-facing method. This can be tricky
    for things like DataFrame.apply which is sometimes used internally. We may need
    to refactor some methods to have a user-facing DataFrame.apply that calls an internal
    DataFrame._apply. The internal method would not call __finalize__, just the
    user-facing DataFrame.apply would.

If you're interested in working on this please post a comment indicating which method
you're working on. Un-xfail the test, then update the method to pass the test. Some of these
will be much more difficult to work on than others (e.g. groupby is going to be difficult). If you're
unsure whether a particular method is likely to be difficult, ask first.

  • DataFrame.__getitem__ with a scalar
  • DataFrame.eval with engine="numexpr"
  • DataFrame.duplicated
  • DataFrame.add, mul, etc. (at least for most things; some work to do on conflicts / overlapping attrs in binops)
  • DataFrame.combine, DataFrame.combine_first
  • DataFrame.update
  • DataFrame.pivot, pivot_table
  • DataFrame.stack
  • DataFrame.unstack
  • DataFrame.explode BUG: added finalize to explode, GH28283 #46629
  • DataFrame.melt BUG/TST/DOC: added finalize to melt, GH28283 #46648
  • DataFrame.diff
  • DataFrame.applymap
  • DataFrame.append
  • DataFrame.merge
  • DataFrame.cov
  • DataFrame.corrwith
  • DataFrame.count
  • DataFrame.nunique
  • DataFrame.idxmax, idxmin
  • DataFrame.mode
  • DataFrame.quantile (scalar and list of quantiles)
  • DataFrame.isin
  • DataFrame.pop
  • DataFrame.squeeze
  • Series.abs
  • DataFrame.get
  • DataFrame.round
  • DataFrame.convert_dtypes
  • DataFrame.pct_change
  • DataFrame.transform
  • DataFrame.apply
  • DataFrame.any, sum, std, mean, etdc.
  • Series.str. operations returning a Series / DataFrame
  • Series.dt. operations returning a Series / DataFrame
  • Series.cat. operations returning a Series / DataFrame
  • All groupby operations (at least some work)
  • .iloc / .loc Add missing __finalize__ calls in indexers/iterators #46101
@TomAugspurger TomAugspurger added this to the 1.0 milestone Sep 4, 2019
@TomAugspurger TomAugspurger added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Sep 4, 2019
@TomAugspurger TomAugspurger changed the title DataFrame.reset_index doesn't call __finalize__ Various methods don't call call __finalize__ Sep 4, 2019
@TomAugspurger
Copy link
Contributor Author

How __finalize__ works with methods like concat requires a bit of discussion. How do we reconcile metadata from multiple sources?

In #27108, I'm using _metadata to propagate whether the NDFrame allows duplicate labels. In this situation, my ideal reconciliation function would be

def reconcile_concat(others: List[DataFrame]) -> bool:
    """
    Allow duplicates only if all the inputs allowed them.

    If any disallow them, we disallow them.
    """
    return all(x.allows_duplicates for x in others)

However, that reconciliation strategy isn't valid / proper for arbitrary metadata. Which I think argues for some kind of dispatch system for reconciling metadata, where the attribute gets to determine how things are handled.

allows_duplicate_meta = PandasMetadata("allows_duplicates")  # the attribute name

@allows_duplicate_meta.register(pd.concat)  # the method
def reconcile_concat():
    ...

Then we always pass method to NDFrame.__finalize__, which we'll use to look up the function for how to reconcile things. A default reconciliation can be provided.

cc @jbrockmendel, since I think you looked into metadata propagation in another issue.

@jbrockmendel
Copy link
Member

IIRC I was looking at _metadata to try to implement units (this predated EAs). One of the biggest problems I had was that metadata on a Series didn't behave well when that Series is inserted into a DataFrame.

Do we have an idea of how often _metadata is used in the wild? i.e. could we deprecate it and make an EA-based implementation?

@TomAugspurger
Copy link
Contributor Author

It’s essentially undocumented, so I’m OK with being aggressive here.

What would an EA-based implementation look like? For something like units, metadata may not be appropriate. I think an EA dtype makes more sense.

I’ll add this to the agenda for next weeks call.

@jbrockmendel
Copy link
Member

What would an EA-based implementation look like?

It's a bit ambiguous what this question is referring to, but big picture something like _metadata but with dispatch logic could be done with something like (for this example I'm thinking of units in physics where (4 meters) / (2 seconds) == 2 m/s):

class MyDtype(ExtensionDtype):
    def __mul__(self, other):
        other = getattr(other, "dtype", other)
        return some_new_dtype


class MyEA(ExtensionArray):
     def __mul__(self, other):
         result = self._data * other
         dtype = self.dtype * other
         return type(self)(result, dtype)

@TomAugspurger
Copy link
Contributor Author

Right. Does the current EA interface suffice for that use case, or are there additional hooks needed?

@TomAugspurger TomAugspurger modified the milestones: 1.0, Contributions Welcome Nov 12, 2019
@TomAugspurger
Copy link
Contributor Author

Not a blocker for 1.0.

@TomAugspurger TomAugspurger added the metadata _metadata, .attrs label Apr 6, 2020
TomAugspurger added a commit to TomAugspurger/pandas that referenced this issue Apr 6, 2020
Progress towards pandas-dev#28283. This
calls `finalize` for all the public series methods where I think it
makes sense.
@TomAugspurger
Copy link
Contributor Author

Do people think that accessor methods like .str and .dt should call finalize? IMO, yes they should.

@jorisvandenbossche
Copy link
Member

But would you then handle name propagation there?

@TomAugspurger
Copy link
Contributor Author

Name propagation isn't (currently) handled in __finalize__. I don't think it should be handled there currently, since the current __finalize__ isn't well suited to resolving the name when there are multiple inputs. In the future it might make sense.

My two motivating use-cases here are

  1. My allow_duplicate_labels PR, for disallowing duplicates
  2. A workload that preserves something like .attrs["source"] = "file.csv" through as many operations as makes sense.

@jorisvandenbossche
Copy link
Member

Name propagation isn't (currently) handled in finalize.

It actually is, not? At least in some cases? (eg for new Series originating from other Series, where other is that Series).

@TomAugspurger
Copy link
Contributor Author

Apologies, I forgot that name was in _metadata. So yes, name handling could be handled there.

@TomAugspurger
Copy link
Contributor Author

When should we call finalize? A high-level list:

Yes

  1. Reshape operations (stack, unstack, reset_index, set_index, to_frame)
  2. Indexing (take, loc, iloc, __getitem__, reindex, drop, assign, select_dtypes, nlargest?)
  3. "transforms" (repeat, explode, shift, diff, round, isin, fillna, isna, dropna, copy, rename, applymap, .transform, sort_values, sort_index)
  4. Accessor methods returning Series .str, .dt, .cat
  5. Binary ops (arithmetic, comparison, combine,
  6. ufuncs
  7. concat / merge / joins, append
  8. cumulative aggregations (cumsum)?

Unsure

  1. Reductions (DataFrame.sum(), etc. count, quantlie, idxmin)
  2. groupby, pivot, pivot_table?
  3. DataFrame.apply?
  4. corr, cov, etc.

These are somewhat arbitrary. I can't really come up with a rule why a reduction like DataFrame.sum() shouldn't call __finalize__ while DataFrame.cumsum does. So perhaps the rule should be "any NDFrame method returning an NDFrame (or Tuple[NDFrame]) will call __finalize__". I like the simplicity of that.

TomAugspurger added a commit to TomAugspurger/pandas that referenced this issue Apr 7, 2020
Progress towards pandas-dev#28283. This
adds tests that ensures `NDFrame.__finalize__` is called in more places.
Thus far I've added tests for anything that meets the following rule:

> Pandas calls `NDFrame.__finalize__` on any NDFrame method that returns
> another NDFrame.

I think that given the generality of `__finalize__`, making any kind of
list of which methods should call it is going to be somewhat arbitrary.
That rule errs on the side of calling it too often, which I think is my
preference.
TomAugspurger added a commit to TomAugspurger/pandas that referenced this issue Apr 7, 2020
Progress towards pandas-dev#28283. This
adds tests that ensures `NDFrame.__finalize__` is called in more places.
Thus far I've added tests for anything that meets the following rule:

> Pandas calls `NDFrame.__finalize__` on any NDFrame method that returns
> another NDFrame.

I think that given the generality of `__finalize__`, making any kind of
list of which methods should call it is going to be somewhat arbitrary.
That rule errs on the side of calling it too often, which I think is my
preference.
TomAugspurger added a commit to TomAugspurger/pandas that referenced this issue Apr 7, 2020
Progress towards pandas-dev#28283. This
adds tests that ensures `NDFrame.__finalize__` is called in more places.
Thus far I've added tests for anything that meets the following rule:

> Pandas calls `NDFrame.__finalize__` on any NDFrame method that returns
> another NDFrame.

I think that given the generality of `__finalize__`, making any kind of
list of which methods should call it is going to be somewhat arbitrary.
That rule errs on the side of calling it too often, which I think is my
preference.
@TomAugspurger
Copy link
Contributor Author

I've updated the original post. Hopefully we can find some contributors interested in working on getting finalize called in more places.

@Sadin
Copy link

Sadin commented Sep 3, 2020

@TomAugspurger I would be interested in contributing to pandas and start by helping to tackle some of these methods. Which methods might be good places to start?

@theOehrly
Copy link
Contributor

.quantile can be ticked off as well. Done as of #47183

@covertg
Copy link

covertg commented Jun 8, 2022

Hi, I'd be happy to help tick some of the boxes off here. Would love to see attrs move out of its experimental status. However, I'm new to contributing to pandas and would appreciate suggestions on which methods to start with.

@hamedgibago
Copy link

Hi, I want to take my first issue and I'm new contributor to pandas. I'd like to get DataFrame.count as my first. Is it good for start? If not please offer another one. Thanks.

@SomtochiUmeh
Copy link
Contributor

take

@SomtochiUmeh
Copy link
Contributor

SomtochiUmeh commented Jul 20, 2022

Starting with Dataframe.idxmax() and Dataframe.idxmin()

mroeschke pushed a commit that referenced this issue Jul 25, 2022
#47821)

* Fixed metadata propagation in Dataframe.idxmax and Dataframe.idxmin #28283

* fixing PEP 8 issues

* removing unnecessary pytest.param()

* removing unnecessary pytest.param
@seljaks
Copy link
Contributor

seljaks commented Sep 7, 2022

Hi, I'm new to open source and I'm interested in contributing to this issue. I'd like to start with DataFrame.add and if that goes well move on to sub, mul, and div

@yuanx749
Copy link
Contributor

Worked on corr and cov.
Skip corrwith for the time being since it involves another NDFrame.

@mroeschke mroeschke removed this from the Contributions Welcome milestone Oct 13, 2022
This was referenced Mar 27, 2023
@bobzhang-stack
Copy link

bobzhang-stack commented Apr 1, 2023

Hello,
I'm new to contributing to open source and am interesting in doing one of the tasks for this issue.
@TomAugspurger Is there any particular method (of the remaining ones still with finalize issue) that would be recommended for me to look into? Is pop alright to start with (it seemed like someone was working on it but I didn't see a pull request for resolving it)?

@TomAugspurger
Copy link
Contributor Author

TomAugspurger commented Apr 1, 2023

@bobzhang-stack it looks like pop might be done.

In [74]: df = pd.DataFrame({"A": [1, 2], "B": [1, 2]})

In [75]: df.attrs['a'] = 1

In [76]: df.pop("B").attrs
Out[76]: {'a': 1}

In [77]: df.attrs
Out[77]: {'a': 1}

It seems the majority of the remaining ones are related to operations between multiple objects with attrs (#49916). Aside from that, there's .eval with engine="numexpr". I'm not sure how hard that would be.

@KartikeyBartwal
Copy link

Hi I'm starting to work on DataFrame.merge

xiaoxiaoimg pushed a commit to xiaoxiaoimg/pandas that referenced this issue May 15, 2024
xiaoxiaoimg pushed a commit to xiaoxiaoimg/pandas that referenced this issue May 16, 2024
@Frostbyte72
Copy link

Frostbyte72 commented Nov 22, 2024

Would it be possible for me to be assigned to df.merge() to attempt a fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests