-
-
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
PERF: first try inplace setitem for .at indexer #49772
PERF: first try inplace setitem for .at indexer #49772
Conversation
@jbrockmendel any other comment, or OK to merge? |
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.
can u add the regressed benchmark
Good point. Unless I am missing something, it seems we actually don't have any benchmarks at all for |
Added a benchmark case for at (both getitem and setitem) to the existing DataFrame indexing benchmark class. Using a code snippet mimicking the benchmark setup:
|
doc/source/whatsnew/v1.5.3.rst
Outdated
@@ -17,6 +17,7 @@ Fixed regressions | |||
- Fixed regression in :meth:`DataFrameGroupBy.transform` when used with ``as_index=False`` (:issue:`49834`) | |||
- Enforced reversion of ``color`` as an alias for ``c`` and ``size`` as an alias for ``s`` in function :meth:`DataFrame.plot.scatter` (:issue:`49732`) | |||
- Fixed regression in :meth:`SeriesGroupBy.apply` setting a ``name`` attribute on the result if the result was a :class:`DataFrame` (:issue:`49907`) | |||
- Fixed performance regression in the :meth:`~DataFrame.at` indexer (:issue:`49771`) |
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.
can you clarify it is setitem and not getitem
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.
Done
This is from the commit that I originally also tested in the PR that caused the perf regression: https://github.com/pandas-dev/pandas/pull/47074/files#r878902106 (but eventually didn't include because it didn't seem needed for getting the correct behaviour, but didn't consider performance at the time)
Using the example from #49729:
This is not yet as fast as on 1.4.x, but #49771 further helps with this as well. With both together, I get to down to 119 ms (vs 50 ms on 1.4.3).
The remaining overhead is mostly from Manager.iget to get the SingleBlockManager
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.