FEAT-#6144: Stop defaulting at API layer for a bunch of methods #6145

vnlitvinov · 2023-05-16T13:33:46Z

What do these changes do?

first commit message and PR title follow format outlined here

NOTE: If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.
passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
signed commit with git commit -s
Resolves Stop defaulting-to-pandas at frontend for a bunch of Series/DataFrame methods #6144
tests added and passing
module layout described at docs/development/architecture.rst is up-to-date

…bunch of Series/DataFrame methods Co-authored-by: mvashishtha <mahesh@ponder.io> Co-authored-by: Naren Krishna <naren@ponder.io> Signed-off-by: Vasily Litvinov <fam1ly.n4me@yandex.ru>

Signed-off-by: Vasily Litvinov <fam1ly.n4me@yandex.ru>

RehanSD

Left a few quick questions but overall LGTM!

RehanSD · 2023-05-16T23:29:14Z

modin/core/storage_formats/base/query_compiler.py

+                df = df.squeeze(axis=1)
+            return df.mask(cond, **kwargs)
+
+        return DataFrameDefault.register(mask)(self, cond, **kwargs)


Why can't we pass in pandas.DataFrame.mask here?

We probably can, I think I've copy-pasted some of my other fix where the behaviour was different... I had already implemented the change locally before your comment, but haven't tested to it wasn't pushed. Should be fixed now.

modin/core/storage_formats/base/query_compiler.py

RehanSD · 2023-05-16T23:30:59Z

modin/core/storage_formats/base/query_compiler.py

+    @doc_utils.add_refer_to("Series.argsort")
+    def argsort(self, **kwargs):  # noqa: PR02
+        """
+        Return the integer indices that would sort the Series values.


Minor nit but why have we added a docstring here?

Our doc checker insists on having docstrings, so we'll have to either silence it with pragmas or fill in at least some docstrings... and for methods which are a direct match for pandas I feel it useful to copy their docstrings for clarity.

RehanSD · 2023-05-16T23:33:14Z

modin/pandas/base.py

-            "mask",
-            cond,
-            other=other,
+        return self._create_or_update_from_compiler(


Why are we using this instead of just the constructor? Or will this method just use the constructor if in place is false, and otherwise do an in place update?

Indeed. It handles the inplace properly.

RehanSD · 2023-05-16T23:36:37Z

modin/pandas/series.py

-        return self._default_to_pandas(pandas.Series.nlargest, n=n, keep=keep)
+        if len(self._query_compiler.columns) == 0:
+            raise NotImplementedError(
+                "Series.nlargest is not implemented for empty Series."


Is this true for pandas as well?

Hmm, just checked, and in pandas it returns Series([], dtype: float64) thingy... should be fixed now.

modin/pandas/base.py

+        **kwargs,
+    ):  # noqa: PR01, RT01, D200
+        return self._create_or_update_from_compiler(
+            self._query_compiler.interpolate(
+                method=method,
+                axis=axis,
+                limit=limit,
+                inplace=False,


RehanSD

Left 2 quick questions - lgtm otherwise!

RehanSD · 2023-05-17T20:25:08Z

modin/pandas/base.py

@@ -1905,7 +1905,6 @@ def mask(
                level=level,
                errors=errors,
                try_cast=try_cast,
-                squeeze_self=not self._is_dataframe,


Why was this argument removed?

I'm assuming this is because mask and pct_change are equivalent in pandas whether applied to a Series or a 1 col DataFrame?

yeah, they seem to behave identically in pandas

RehanSD · 2023-05-17T20:25:42Z

modin/pandas/series.py

-        return super().mask(cond, other, inplace, axis, level, errors, try_cast)
+        # This method exists because `errors` has different default value for Series :(
+        return super(Series, self).mask(
+            cond, other, inplace, axis, level, "raise", try_cast


Shouldn't we pass in errors here?

Also seems this argument has been deprecated since 1.5? So we should be coercing types anyways?

That deprecation is the reason why I'm not passing the errors through, as it is expected to always behave as if raise is given.

RehanSD

LGTM

mvashishtha and others added 10 commits May 16, 2023 15:08

FEAT-modin-project#6144: Stop defaulting-to-pandas at frontend for a …

61a80bd

…bunch of Series/DataFrame methods Co-authored-by: mvashishtha <mahesh@ponder.io> Co-authored-by: Naren Krishna <naren@ponder.io> Signed-off-by: Vasily Litvinov <fam1ly.n4me@yandex.ru>

Fix Series.mask signature

27179d6

Implement QC.align() defaulting to pandas

274ecc1

Signed-off-by: Vasily Litvinov <fam1ly.n4me@yandex.ru>

Implement BaseQC.mask defaulting to pandas

6ed5043

Signed-off-by: Vasily Litvinov <fam1ly.n4me@yandex.ru>

Implement BaseQC.pct_change defaulting to pandas

ea4f92f

Signed-off-by: Vasily Litvinov <fam1ly.n4me@yandex.ru>

Fix .align impl

ab16c5c

Fix Series.argsort

d79de6b

Implement BaseQC.asfreq defaulting to pandas

f7ea7dd

Signed-off-by: Vasily Litvinov <fam1ly.n4me@yandex.ru>

Undo .reindex_like change, handled separately

e9b2b04

Fix Series.divmod and .rdivmod

fb5da43

vnlitvinov added the ponder modin work desired for the ponder product label May 16, 2023

vnlitvinov added 5 commits May 16, 2023 17:40

Restore ewm defaulting to pandas

01068a1

Fix interpolate, sizeof and swaplevel, revert Series.hist

113f7d6

Revert the change for Base._string_function

325f74c

Fix Series.swaplevel

9ee3c1e

Fix in-place masking

1f19217

vnlitvinov changed the title ~~Stop defaulting at API layer for a bunch of methods~~ FEAT-#6144: Stop defaulting at API layer for a bunch of methods May 16, 2023

RehanSD reviewed May 16, 2023

View reviewed changes

RehanSD mentioned this pull request May 17, 2023

FIX-#6107: Allow pass through of tz_convert and tz_localize to QC if possible #6137

Merged

7 tasks

vnlitvinov added 3 commits May 17, 2023 09:28

Fix doc checker

fee4697

Simplify .mask and .pct_change API, fix .nlargest and .nsmallest

21d2ccc

Trying to fix HDK test failure

56d6fb2

github-advanced-security bot found potential problems May 17, 2023

View reviewed changes

vnlitvinov marked this pull request as ready for review May 17, 2023 09:26

vnlitvinov requested a review from a team as a code owner May 17, 2023 09:26

RehanSD reviewed May 17, 2023

View reviewed changes

RehanSD approved these changes May 17, 2023

View reviewed changes

RehanSD merged commit 53a04b8 into modin-project:master May 17, 2023
56 checks passed

vnlitvinov deleted the stop-frontend-default-1 branch May 18, 2023 14:26

mvashishtha mentioned this pull request Mar 19, 2024

REFACTOR: Remove all defaults to pandas at API layer. #7104

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT-#6144: Stop defaulting at API layer for a bunch of methods #6145

FEAT-#6144: Stop defaulting at API layer for a bunch of methods #6145

vnlitvinov commented May 16, 2023

RehanSD left a comment

RehanSD May 16, 2023

vnlitvinov May 17, 2023

RehanSD May 16, 2023

vnlitvinov May 17, 2023

RehanSD May 16, 2023

vnlitvinov May 17, 2023

RehanSD May 16, 2023

vnlitvinov May 17, 2023 •

edited

RehanSD left a comment

RehanSD May 17, 2023

RehanSD May 17, 2023

vnlitvinov May 18, 2023

RehanSD May 17, 2023

RehanSD May 17, 2023

vnlitvinov May 18, 2023

RehanSD left a comment

FEAT-#6144: Stop defaulting at API layer for a bunch of methods #6145

FEAT-#6144: Stop defaulting at API layer for a bunch of methods #6145

Conversation

vnlitvinov commented May 16, 2023

What do these changes do?

RehanSD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnlitvinov May 17, 2023 • edited

Choose a reason for hiding this comment

RehanSD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RehanSD left a comment

Choose a reason for hiding this comment

vnlitvinov May 17, 2023 •

edited