Repository navigation
fix: return null from mean_horizontal for an all-null row on PyArrow, DuckDB, Ibis and Spark-like - #4059
fix: return null from mean_horizontal for an all-null row on PyArrow, DuckDB, Ibis and Spark-like#4059SausageMan99 wants to merge 1 commit into
mean_horizontal for an all-null row on PyArrow, DuckDB, Ibis and Spark-like#4059Conversation
…w, DuckDB, Ibis and Spark-like Closes narwhals-dev#4056 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk | ⚪ Minimal · up to
|
| nw.col("mean"), is_null=nw.col("mean").is_null() | ||
| ) | ||
| expected = {"mean": [2.0, None], "is_null": [False, True]} | ||
| assert_equal_data(result, expected) |
There was a problem hiding this comment.
Could we extend this a bit?
- an integer case, since that takes a different division path on several backends;
- a check that the output is still
Float64when every row is null, as in Polars.
This needs import pytest at the top of the file. The filter is there to get an all-null column that keeps its dtype: building all-None columns directly fails on Ibis.
| assert_equal_data(result, expected) | |
| @pytest.mark.parametrize( | |
| "data", [{"a": [1.0, None], "b": [3.0, None]}, {"a": [1, None], "b": [3, None]}] | |
| ) | |
| def test_meanh_all_null_row_is_null( | |
| constructor: Constructor, data: dict[str, list[float | None]] | |
| ) -> None: | |
| df = nw.from_native(constructor(data)) | |
| mean = nw.mean_horizontal("a", "b") | |
| result = df.select(mean=mean, is_null=mean.is_null()) | |
| expected = {"mean": [2.0, None], "is_null": [False, True]} | |
| assert_equal_data(result, expected) | |
| result = df.filter(nw.col("a").is_null()).select(mean=mean) | |
| assert result.collect_schema() == {"mean": nw.Float64()} |
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks for the PR @SausageMan99, and nice catch extending the fix to Ibis and Spark-like beyond what the issue reported!
Two things before we can merge:
1. pandas-like still returns NaN on pandas<3 with nullable and pyarrow-backed dtypes. The new test fails on pandas[nullable] and pandas[pyarrow] with pandas 2.x (I checked 2.2.3 and 2.3.3): on those dtypes 0 / 0 gives NaN, which is not null. pandas 3 returns <NA> there, which is likely why it passed for you locally. CI runs these constructors on pandas 2.x, so it would fail there.
#4060 had the fix for this one, in src/narwhals/_pandas_like/namespace.py::mean_horizontal:
non_na = (1 - s.is_null() for s in expr_results)
count = reduce(operator.add, non_na)
return [(reduce(operator.add, series) / count).zip_with(count > 0, None)]Could you port it, and credit @ioseba with a trailer in the commit message?
Co-authored-by: I. Alonso <11636356+ioseba@users.noreply.github.com>
2. Test: pin the output dtype. See the inline comment. When every row is null, the output should still be Float64, as in Polars. That already holds on every backend with this PR, and the test should lock it in.
Optional: now that all backends agree, we could make the guarantee explicit in the docstring, in both src/narwhals/functions.py and src/narwhals/stable/v2/__init__.py:
Null values are ignored. If all values in a row are null, the result is null.
With both changes, the test passes locally on pandas, pandas[nullable], pandas[pyarrow], polars (eager and lazy), pyarrow, duckdb, sqlframe, ibis, dask and modin.
Description
For a row where every input is null, the count of non-null values is 0, so
mean_horizontalcomputed0 / 0and returned NaN on PyArrow, DuckDB, Ibis and Spark-like, where NaN is not null. It now returns null there, like pandas and Polars.The issue mentions PyArrow and DuckDB; the new test showed the same result on Ibis and SQLFrame, so those are fixed too.
What type of PR is this? (check all applicable)
Related issues
AI assistance
Checklist
pytestpassing locally (not CI):Local run (template command + ibis)
🤖 Generated with Claude Code
Summary by CodeRabbit