[go: nahoru, domu]

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

BUG Validate transformers in transform #17360

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 glemaitre added this to TO BE MERGED in Guillaume's pet Jun 5, 2020
@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

@glemaitre glemaitre merged commit 24e7be0 into scikit-learn:0.23.X Aug 3, 2020
@glemaitre glemaitre moved this from TO BE MERGED to MERGED in Guillaume's pet Aug 4, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants