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

[SUGGESTION] Drop the column and remainder parameters from RepeatingBasisFunction #458

Open
Garve opened this issue Apr 20, 2021 · 4 comments
Labels
bug Something isn't working

Comments

@Garve
Copy link
Contributor

Garve commented Apr 20, 2021

Hi!

Maybe it has historical reasons, but do we need the parameters column and remainder? Because this can be handled easily and more general using a ColumnTransformer.

Best
Robert

@Garve Garve added the bug Something isn't working label Apr 20, 2021
@Garve
Copy link
Contributor Author

Garve commented Apr 20, 2021

(Sorry, not a bug.)

@koaning
Copy link
Owner

koaning commented Apr 21, 2021

You're suggesting something sensible here, but part of me is a little anxious about introducing a breaking change without adding a useful feature. I think this feature is in production in a bunch of places so I want to be a bit conservative here. There's certainly a sense of "purity" missing with the two parameters, but practically it doesn't feel like there's that much harm in keeping them as is.

Also, if you're encoding a single column, then technically you can already combine it with a ColumnTransformer, right?

@MBrouns
Copy link
Collaborator

MBrouns commented Apr 21, 2021

I'd argue the feature in this case would be consistency with the main sklearn API. When we built this transformer either the ColumnTransformer wasn't out yet or was still experimental, but now it's been there long enough that it's clear that that is the way forward.

I'd vote for deprecating the current behaviour with a proper warning and suggestion on how to change it and then making the change a couple of releases down the road

@koaning
Copy link
Owner

koaning commented Apr 21, 2021

@MBrouns fair enough! We're at version 0.6.6 now, so we might want to change the behavior in 0.9.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants