Skip to content

ENH: Use Kahan summation to calculate groupby.sum() #38903

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

Merged
merged 6 commits into from
Jan 3, 2021

Conversation

phofl
Copy link
Member

@phofl phofl commented Jan 2, 2021

This simplifies the op example.

I think

series = Series([1e16, 99, -5e15, -5e15])
series.sum()

is dispatched to numpy? The result is wrong too. 100.0

Mean doesn't work either. I will look through the functions to determine which need Kahan summation too and open an issue to track these.

@phofl phofl added the Groupby label Jan 2, 2021
@phofl
Copy link
Member Author

phofl commented Jan 2, 2021

We dispatch to numpy in case of 32bit systems, which returns the wrong result

@@ -51,6 +51,7 @@ Other enhancements
- :func:`pandas.read_sql_query` now accepts a ``dtype`` argument to cast the columnar data from the SQL database based on user input (:issue:`10285`)
- Improved integer type mapping from pandas to SQLAlchemy when using :meth:`DataFrame.to_sql` (:issue:`35076`)
- :func:`to_numeric` now supports downcasting of nullable ``ExtensionDtype`` objects (:issue:`33013`)
- Improve numerical stability for :meth:`DataFrameGroupBy.sum()` and :meth:`SeriesGroupBy.sum()` through using Kahan summation (:issue:`38778`)
Copy link
Member

Choose a reason for hiding this comment

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

no brackets after ...GroupBy.sum?

Copy link
Member Author

Choose a reason for hiding this comment

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

thx

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

comments, ping on green.

note that we have been seeing some recent work on combing the aggregation functions for 1d and groupby, this would be a great case here. (slightly complicated because for 1d we actually dispatch to bottleneck if installed), otherwise we use the np.nan* functions so have to see if this is worth it).

@@ -51,6 +51,7 @@ Other enhancements
- :func:`pandas.read_sql_query` now accepts a ``dtype`` argument to cast the columnar data from the SQL database based on user input (:issue:`10285`)
- Improved integer type mapping from pandas to SQLAlchemy when using :meth:`DataFrame.to_sql` (:issue:`35076`)
- :func:`to_numeric` now supports downcasting of nullable ``ExtensionDtype`` objects (:issue:`33013`)
- Improve numerical stability for :meth:`DataFrameGroupBy.sum` and :meth:`SeriesGroupBy.sum` through using Kahan summation (:issue:`38778`)
Copy link
Contributor

Choose a reason for hiding this comment

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

move to group bug fixes

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved

@jreback jreback added this to the 1.3 milestone Jan 3, 2021
@phofl
Copy link
Member Author

phofl commented Jan 3, 2021

@jreback green

@jreback jreback merged commit 5cc24c2 into pandas-dev:master Jan 3, 2021
@jreback
Copy link
Contributor

jreback commented Jan 3, 2021

thanks @phofl

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

Successfully merging this pull request may close these issues.

BUG: sum vs groupby.sum errors
3 participants