-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
ENH: Support third-party execution engines in Series.map #61467
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
ENH: Support third-party execution engines in Series.map #61467
Conversation
@@ -20,6 +20,7 @@ | |||
timedelta_range, | |||
) | |||
import pandas._testing as tm | |||
from pandas.tests.apply.conftest import engine # noqa: F401 |
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.
Curious why this needs importing since it's already in the conftest.py
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.
Good question. I answered in a comment, so readers of that input don't need to ask themselves it. Most tests related to apply/map are in tests/apply
, so the fixture is defined there. But it's also useful here in tests/series/methods/
. test/apply/conftest.py
is not in scope when running tests/series/methods
, so I need to import manually in order to use it. Another alternative would be to move the fixture to the global conftest.py
, but I think this approach keeps things better organized and simple.
I moved the mock classes to conftest.py
as suggested, thanks for the review.
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 think if your work in #61125 continues to expand, I would be OK moving this to the global conftest.py
pandas/tests/apply/common.py
Outdated
from pandas.core.groupby.base import transformation_kernels | ||
|
||
# There is no Series.cumcount or DataFrame.cumcount | ||
series_transform_kernels = [ | ||
x for x in sorted(transformation_kernels) if x != "cumcount" | ||
] | ||
frame_transform_kernels = [x for x in sorted(transformation_kernels) if x != "cumcount"] | ||
|
||
|
||
class MockExecutionEngine(BaseExecutionEngine): |
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.
Could we move this to the conftest.py
as well? In the past, I've moved away from defining objects in non test_
or conftest.py
files
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.
Looks pretty good. Just a few questions
map
andapply
methods #61125doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.