Skip to content
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: allow for scalars in sum_horizontal #1868

Open
MarcoGorelli opened this issue Jan 26, 2025 · 3 comments
Open

enh: allow for scalars in sum_horizontal #1868

MarcoGorelli opened this issue Jan 26, 2025 · 3 comments
Labels
enhancement New feature or request low priority

Comments

@MarcoGorelli
Copy link
Member

e.g.

import duckdb
import narwhals as nw
rel = duckdb.sql('select * from values (1, 4), (1, 2), (2, 3), (2, 4) df(a, b)')
df = nw.from_native(rel)
 df.select(nw.sum_horizontal(1, 'a', 'b'))

throws

InvalidIntoExprError: Expected an object which can be converted into an expression, got <class 'int'>

Hint:
- if you were trying to select a column which does not have a string
  column name, then you should explicitly use `nw.col`.
  For example, `df.select(nw.col(0))` if you have a column named `0`.
- if you were trying to create a new literal column, then you
  should explicitly use `nw.lit`.
  For example, `df.select(nw.lit(0))` if you want to create a new
  column with literal value `0`.

I think we should have a way of allowing for anything which can't be parsed as an expression to be parsed as nw.lit in these cases

@MarcoGorelli MarcoGorelli added the enhancement New feature or request label Jan 26, 2025
@FBruzzesi
Copy link
Member

FBruzzesi commented Jan 29, 2025

Are we aiming for the same behavior for boolean's (e.g. in any_horizontal and all_horizontal)?

Additionally, polars allow to pass numbers to (any|all)_horizontal and booleans to (sum|mean|max|min)_horizontal. Thoughts on this?

I have a way to address it, which is failing for dask only (I believe due to some broadcasting issues for how we create the lit)

@MarcoGorelli
Copy link
Member Author

MarcoGorelli commented Jan 29, 2025

thanks for looking into it!

not sure, will think about it, but this probably is quite low-prio for now, and may require a bit of a refactor if we want to do it properly (and not just special-case a few functions)

@FBruzzesi
Copy link
Member

this probably is quite low-prio for now

I agree, using nw.lit(value) should work anyway, and that's what the error message suggest to do.

may require a bit of a refactor if we want to do it properly (and not just special-case a few functions)

I was able to tackle it by adding a few extra checks/cases in extract_compliant function, however for different methods and backend we might require casting to the correct type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request low priority
Projects
None yet
Development

No branches or pull requests

2 participants