Skip to content

Fix sum() axis functionality - #263

Open
juaristi22 wants to merge 1 commit into
mainfrom
maria/fix_sum_method
Open

Fix sum() axis functionality#263
juaristi22 wants to merge 1 commit into
mainfrom
maria/fix_sum_method

Conversation

@juaristi22

Copy link
Copy Markdown
Collaborator

Fix #262

@MaxGhenis

Copy link
Copy Markdown
Collaborator

Heads-up: I opened #283 to fix a related-but-distinct bug — weighted sum() silently returns 0.0 on a MicroSeries/MicroDataFrame with a non-default index (because set_weights stored the weights Series with a default RangeIndex). The fix touches microseries.py set_weights and the MicroDataFrame set_weights, so the diff shouldn't conflict with #263 (which is about sum(axis=...)). Flagging in case of merge-conflict-detective-work later.

@vahid-ahmadi vahid-ahmadi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good fix, and it quietly repairs more than the title suggests: before this, mdf.sum(axis=0) also returned an empty Series, because axis=0 was forwarded into MicroSeries.sum() (which takes no arguments), raised TypeError, and got swallowed by the except Exception: pass. Stripping axis out of ms_kwargs fixes that too. The test coverage on skipna/min_count/axis="columns" is thorough.

One real gap and a couple of smaller notes.

1. Positional axis is still broken — df.sum(1) silently returns an empty Series.

axis = kwargs.get("axis", 0)

pandas fully supports df.sum(1), and it's a common shorthand. With a positional argument, kwargs is empty, so axis reads as 0, the column-wise branch runs, and getattr(self[col], "sum")(1) raises TypeError against MicroSeries.sum(self) — caught by except Exception: pass, every column dropped, empty Series returned. Same failure mode you're fixing, just via the other calling convention.

Worth pulling axis out of the positional slot:

def fn(*args, **kwargs):
    if args and "axis" not in kwargs:
        axis, *rest = args
        args = tuple(rest)
    else:
        axis = kwargs.get("axis", 0)
    ms_kwargs = {k: v for k, v in kwargs.items() if k != "axis"}

That also removes a latent problem in the axis=1 branch: getattr(df, name)(axis=1, *args, **ms_kwargs) is legal Python but raises TypeError: got multiple values for argument 'axis' the moment args is non-empty, since axis is pandas' first positional parameter.

2. axis=1 reaches functions pandas doesn't have.

SCALAR_FUNCTIONS is derived from every MicroSeries method with _rtype == float (microseries.py:821-826), which includes gini, top_10_pct_share, top_x_pct_share, t10_b50, and friends. getattr(pd.DataFrame, "gini") doesn't exist, so mdf.gini(axis=1) now raises a bare AttributeError naming DataFrame — confusing, given the user called a method on a MicroDataFrame. A row-wise gini is meaningless anyway, so an explicit refusal would read better:

if not hasattr(pd.DataFrame, name):
    raise TypeError(f"{name}() does not support axis=1 on a MicroDataFrame.")

3. Minor

  • The docstring on _create_scalar_function was dropped in the rewrite; the surrounding methods (_create_vector_function, _create_agnostic_function) still carry theirs.
  • The return annotation is Union[pd.Series, float], but no path returns a float.
  • axis=None (explicit) falls into the else and raises ValueError, where pandas treats it as "reduce over everything". Probably fine to leave, just noting it's now a hard error rather than the old silent-empty.

Also — GitHub currently shows this as conflicting with main, so it'll need a rebase before it can go in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DataFrame sum function (possible also Series) does not respect axis arg

3 participants