Skip to content

BUG Validate transformers in transform #17360

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

Merged

Conversation

thomasjpfan
Copy link
Member

Fixes #17355

@thomasjpfan thomasjpfan added this to the 0.23.2 milestone May 26, 2020
Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@glemaitre
Copy link
Member

ping @rth @jeremiedbb @NicolasHug

This is an easy hanging fruit.

@glemaitre
Copy link
Member

ping @adrinjalali I saw is around as well :)

@adrinjalali
Copy link
Member

Should this also have a what's new entry?

@glemaitre
Copy link
Member

True since this is a FIX

@thomasjpfan
Copy link
Member Author

Note this PR targets the 0.23.X branch since it is adding a feature ontop of a removed feature in master. If I add the whats new in 0.23.rst, it would update the docs and it would be a little strange?

Looking back at this, I think this will introduce a bug for users that want to construct a prefitted FeatureUnion. When transforming the validation should only check if the estimator has "transform".

@adrinjalali
Copy link
Member

Oh, yhis is why I prefer to remove the deprecations much closer to the next major release rather than right after a release.

In this case, we can keep this PR open and add it to the minor release milestone, have a separate PR which adds the whats_new entry to master and backport it to the release branch at the time of the release, and merge this one right before we'd like to release I suppose.

@glemaitre
Copy link
Member

ping @adrinjalali :P

@adrinjalali
Copy link
Member

I'm not sure what I'm pinged for @glemaitre :P I'm happy to have this merged right before we do the next minor release. We also need the separate PR for the changelog to the master branch.

I'm also happy if we do a minor release in a week. There's a ton of documentation improvement which we could improve in the release, and a few fixes, but I guess not too many.

@glemaitre
Copy link
Member

I'm also happy if we do a minor release in a week. There's a ton of documentation improvement which we could improve in the release, and a few fixes, but I guess not too many.

I think this is the last flagged PR for 0.23.2. I merged a fix for the StackingEstimator which should also go in 0.23.2.
It might be time to make a new release.

@adrinjalali
Copy link
Member

This PR has to be merged independent of the other ones which will go to the release PR since it targets the release branch directly and not the master branch. So I'd say if you wanna do a release, prepare that PR, and close to the release merge this one and that one? And again, remember the changelog entry ;)

@thomasjpfan
Copy link
Member Author

The whats new update is at #17912

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants