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.

Sign up for GitHub

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

Jump to bottom

Agnostic ColumnSelector #659

Merged
merged 5 commits into from
May 8, 2024
Merged

Agnostic ColumnSelector #659

merged 5 commits into from
May 8, 2024

Conversation

anopsy
Copy link
Contributor

@anopsy anopsy commented May 8, 2024 •
edited
Loading

Description

Narwhalyfied ColumnSelector, I added an example with polars df to the docstrings and adjusted the test_columnselector.py
I tried to test the docstrings, but it doesn't work. Maybe we could add those to sklego in the future?

Fixes #658 - partly

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the style guidelines (ruff)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (also to the readme.md)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added tests to check whether the new feature adheres to the sklearn convention
  • New and existing unit tests pass locally with my changes

If you feel your PR is ready for a review, ping @FBruzzesi or @koaning.

@anopsy anopsy changed the title Agnostic transformers Agnostic ColumnSelector May 8, 2024
MarcoGorelli
MarcoGorelli reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@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.

that was fast! just left some initial comments ahead of maintainers' reviews

sklego/preprocessing/pandastransformers.py
"shoesize": [42, 44, 45]
})

ColumnSelector(["length", "shoesize"]).fit_transform(df_pl)
Copy link
Contributor

Choose a reason for hiding this comment

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

what you show below here doesn't look like Polars output, I think that may need updating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me fix that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
So I used polars output but it renders this way in mkdocs:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I think it's just a font that needs setting, the default one isn't exactly monospace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that something I can fix or not really @FBruzzesi ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I never rendered that! @MarcoGorelli send help 😂!

Copy link
Contributor

Choose a reason for hiding this comment

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

😄 dunno, in Narwhals I just have

theme:
  name: material
  font: false

Copy link
Owner

Choose a reason for hiding this comment

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

I recall doing stuff like that in the past as well, it's a monofont isnt always mono kind of a thing. Especially when you add emojis or fancy kinds of unicode to the mix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
looks great now! I'll push it then

sklego/preprocessing/pandastransformers.py Outdated Show resolved Hide resolved
sklego/preprocessing/pandastransformers.py Outdated Show resolved Hide resolved
@anopsy
Copy link
Contributor Author

anopsy commented May 8, 2024

@MarcoGorelli Sorry, I removed your review about fit, I thought I can show you the fix with "Resolve conversation". Anyway now it looks like that:
image

FBruzzesi
FBruzzesi approved these changes May 8, 2024
View reviewed changes
Copy link
Collaborator

@FBruzzesi FBruzzesi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @anopsy ! I start to like this a lot!

NiceGIF

P.S. Test do not run because it is not a PR to main branch, but I can confirm that it is all good (locally):
image

@FBruzzesi FBruzzesi merged commit fe691b5 into koaning:narwhals-development May 8, 2024
1 check passed
@FBruzzesi FBruzzesi mentioned this pull request May 8, 2024
Closed
@anopsy
Copy link
Contributor Author

anopsy commented May 8, 2024

Dude I've run those tests like 13 times, and also full tests twice (btw you can make lunch while they're running, it takes so long) . Thank you team for the support! I'm so excited

@FBruzzesi
Copy link
Collaborator

@anopsy taking this to another level
image

@koaning
Copy link
Owner

koaning commented May 8, 2024

One thing about cleaning prints, did we not configure ruff to check for that?

@anopsy
Copy link
Contributor Author

anopsy commented May 8, 2024

Could we also want some doctests? Or that's too boujee 😅

@FBruzzesi FBruzzesi mentioned this pull request May 18, 2024
Merged
koaning pushed a commit that referenced this pull request May 24, 2024
@FBruzzesi @MarcoGorelli
@anopsy @DeaMariaLeon
feat: Narwhals for dataframe-agnostic codebase (#671)
fbb8e57 
* placeholder to develop narwhals features

* feat: make `ColumnDropper` dataframe-agnostic (#655)

* feat: make ColumnDropped dataframe-agnostic

* use narwhals[polars] in pyproject.toml, link to list of supported libraries

* note that narwhals is used for cross-dataframe support

* test refactor

* docstrings

---------

Co-authored-by: FBruzzesi <[email protected]>

* feat: make ColumnSelector dataframe-agnostic (#659)

* columnselector with test rufformatted

* adding whitespace

* fixed the fit and transform

* removed intendation in examples

* font:false

* feat: make `add_lags` dataframe-agnostic (#661)

* make add_lags dataframe-agnostic

* try getting tests to run?

* patch: cvxpy 1.5.0 support (#663)

---------

Co-authored-by: Francesco Bruzzesi <[email protected]>

* Make `RegressionOutlier` dataframe-agnostic (#665)

* make regression outlier df-agnostic

* need to use eager-only for this one

* pass native to check_array

* remove cudf, link to check_X_y

* feat: Make InformationFilter dataframe-agnostic

* Make Timegapsplit dataframe-agnostic (#668)

* make timegapsplit dataframe-agnostic

* actually, include cuDF

* feat: make FairClassifier data-agnostic (#669)

* start all over

* fixture working

* wip

* passing tests - again

* pre-commit complaining

* changed fixture on test_demographic_parity

* feat: Make PandasTypeSelector selector dataframe-agnostic (#670)

* make pandas dtype selector df-agnostic

* bump version

* 3.8 compat

* Update sklego/preprocessing/pandastransformers.py

Co-authored-by: Francesco Bruzzesi <[email protected]>

* fixup pyproject.toml

* unify (and test!) error message

* deprecate

* update readme

* undo contribution.md change

---------

Co-authored-by: Francesco Bruzzesi <[email protected]>

* format typeselector and bump version

* feat: Make grouped and hierarchical dataframe-agnostic (#667)

* feat: make grouped and hierarchical dataframe-agnostic

* add pyarrow

* narwhals grouped_transformer

* grouped transformer eureka

* hierarchical narwhalified

* so close but so far

* return series instead of DataFrame for y

* grouped WIP

* merge branch and fix grouped

* future annotations

* format

* handling negative indices

* solve conflicts

* hacking C

* fairness: change C values in tests

---------

Co-authored-by: Marco Edward Gorelli <[email protected]>
Co-authored-by: Magdalena Anopsy <[email protected]>
Co-authored-by: Dea María Léon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcoGorelli MarcoGorelli MarcoGorelli left review comments

@koaning koaning koaning left review comments

@FBruzzesi FBruzzesi FBruzzesi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anopsy @FBruzzesi @koaning @MarcoGorelli