Skip to content

feat: mean_horizontal #843

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 2 commits into from
Aug 22, 2024
Merged

feat: mean_horizontal #843

merged 2 commits into from
Aug 22, 2024

Conversation

FBruzzesi
Copy link
Member

@FBruzzesi FBruzzesi commented Aug 22, 2024

What type of PR is this? (check all applicable)

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below.

As mean can only be numeric, unlike #825, it is safe to fill nulls with zeros for the sum, and count the number of non-nulls summands.

@github-actions github-actions bot added enhancement New feature or request internal labels Aug 22, 2024
@FBruzzesi
Copy link
Member Author

Ok, so this ended up being a bit more than initially expected on the polars side 😂

@@ -85,11 +88,28 @@ def lit(self, value: Any, dtype: dtypes.DType | None = None) -> PolarsExpr:
return PolarsExpr(pl.lit(value, dtype=narwhals_to_native_dtype(dtype)))
return PolarsExpr(pl.lit(value))

def mean(self, *column_names: str) -> Any:
def mean(self, *column_names: str) -> PolarsExpr:
Copy link
Member Author

Choose a reason for hiding this comment

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

We should return PolarsExpr to be able to keep chaining and interacting with other narwhals exprs if needed

n_non_zero = reduce(
lambda x, y: x + y, ((1 - e.is_null()) for e in polars_exprs)
)
return PolarsExpr(total._native_expr / n_non_zero._native_expr)
Copy link
Member Author

Choose a reason for hiding this comment

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

__div__ is not currently implemented and I did not want to make the PR even larger

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

this is wonderful, thanks @FBruzzesi !

@MarcoGorelli MarcoGorelli merged commit 18d0dcb into main Aug 22, 2024
26 checks passed
@FBruzzesi FBruzzesi deleted the feat/mean_horizontal branch August 22, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enh]: support mean_horizontal
3 participants