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

fix: fixing unnesscary raise of mean_horizontal #1082

Merged
merged 9 commits into from
Oct 4, 2024

Conversation

Cheukting
Copy link
Contributor

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.

See #1081

@github-actions github-actions bot added the fix label Sep 27, 2024
@Cheukting
Copy link
Contributor Author

Cheukting commented Sep 27, 2024

mean_horizontal was implemented in Polars 0.20.8: https://github.com/pola-rs/polars/releases/tag/py-0.20.8

I found the implementation in Narwhals and it seems need an update as well, will do that in a commit

Copy link
Member

@FBruzzesi FBruzzesi left a comment

Choose a reason for hiding this comment

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

Hey @Cheukting thanks for addressing this!

Probably it's an obsession of mine, but can we keep using generator comprehensions instead of list and for loops?

@Cheukting
Copy link
Contributor Author

All checks have passed @MarcoGorelli ship it! (What cute animal do we have time?)

@FBruzzesi are you sure you want me to refactor them? Because in the for loop there is already a list comprehension, it will become a list comprehension within a list comprehension... it will not be very readable (IMHO)... here is a preview example:

            series = [
                series.extend([_series.fill_null(0) for _series in _expr._call(df)]) for _expr in parsed_exprs
            ]

I can refactor with no problem if it is preferred. Just wanna double check.

@FBruzzesi
Copy link
Member

FBruzzesi commented Sep 29, 2024

Hey hey @Cheukting

All checks have passed @MarcoGorelli ship it! (What cute animal do we have time?)

Awesome! I will let Marco pick animal(s) πŸ˜‚

are you sure you want me to refactor them? Because in the for loop there is already a list comprehension, it will become a list comprehension within a list comprehension... it will not be very readable (IMHO)

I did the same in #1090:

series = (s for _expr in parsed_exprs for s in _expr._call(df))

I know it is nested, but I believe it is still very readable for anyone who has some python experience.

Actually I think this PR should take #1090 into account and set

- output_names=parsed_exprs[0]._output_names,
+ output_names=reduce_output_names(parsed_exprs),

in mean_horizontal as well, otherwise we can end up in a similar issue to what's addressed there

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.

Thanks @Cheukting , and thanks @FBruzzesi for reviewing!

love this

@MarcoGorelli MarcoGorelli merged commit bb493c6 into narwhals-dev:main Oct 4, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: nw.mean_horizontal(nw.all()) raises unnecessarily
3 participants