10000 don't change self.n_values in OneHotEncoder.fit by amueller · Pull Request #12286 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

don't change self.n_values in OneHotEncoder.fit #12286

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

amueller
Copy link
Member
@amueller amueller commented Oct 4, 2018

via #8022.
ping @jorisvandenbossche
Not sure if it's possible to trigger a bug with this but it's definitely not allowed

@amueller amueller added this to the 0.20.1 milestone Oct 4, 2018
@jorisvandenbossche
Copy link
Member

I was looking in the history of commits of #10523, and at a certain point I had it as self._n_values, but for some reason I changed it back to self.n_values .. (and don't know any more why ... ).
Will try to look into it in more detail tomorrow. It might just be it was just an oversight at the time.

In any case, looking at the code and diff of _handle_deprecations here in this PR, I suppose with the changes, you will now get a warning on each fit. While before, only on the first fit (and also for other deprecations, I ensured to only do it on the first fit, eg to not have it too noisy when doing CV).
Now, not that is not easily solvable (so that is certainly not the reason I changed the n_values attribute itself). For that, I think you need to use _n_values instead of n_values at this line: https://github.com/scikit-learn/scikit-learn/pull/12286/files#diff-d12408664448c94dbd880579e1b2e4d9R320

@amueller
Copy link
Member Author
amueller commented Oct 5, 2018

Line 320? But I just set them to be the same thing?
I figured the multiple deprecations might have been your motivation. But in CV this will still raise every time, right? Because they are cloned?

@amueller
Copy link
Member Author

@jorisvandenbossche any chance you had time to look into this?

@jorisvandenbossche
Copy link
Member

@amueller sorry for the slow reply. I didn't really look much further in detail into it, but I also can't directly think of a problem / reason why I didn't do this. So go ahead.

@@ -304,7 +304,7 @@ def n_values_(self):
return self._n_values_

def _handle_deprecations(self, X):

self._n_values = self.n_values
Copy link
Member

Choose a reason for hiding this comment

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

I would put this under the line below, as this is also an internal version of a keyword to handle the deprecation

move private variable below comment
@amueller
Copy link
Member Author

thanks, done!

@amueller
Copy link
Member Author

@jnothman @ogrisel might be good for 0.20.1 and should be pretty straight-forward?

Copy link
Member
@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Looks fine. Add a small what's new to 0.20.1?

@jorisvandenbossche
Copy link
Member

Add a small what's new to 0.20.1?

It's not that much of a user-facing change, so personally not sure it is worth cluttering the what's new with it.

@jnothman
Copy link
Member
jnothman commented Oct 30, 2018 via email

@qinhanmin2014 qinhanmin2014 merged commit eb36e49 into scikit-learn:master Nov 11, 2018
thoo added a commit to thoo/scikit-learn that referenced this pull request Nov 12, 2018
…ybutton

* upstream/master:
  FIX remove FutureWarning in _object_dtype_isnan and add test (scikit-learn#12567)
  DOC Add 's' to "correspond" in docs for Hamming Loss. (scikit-learn#12565)
  EXA Fix comment in plot-iris-logistic example (scikit-learn#12564)
  FIX stop words validation in text vectorizers with custom preprocessors / tokenizers (scikit-learn#12393)
  DOC Add skorch to related projects (scikit-learn#12561)
  MNT Don't change self.n_values in OneHotEncoder.fit (scikit-learn#12286)
  MNT Remove unused assert_true imports (scikit-learn#12560)
  TST autoreplace assert_true(...==...) with plain assert (scikit-learn#12547)
  DOC: add a testimonial from JP Morgan (scikit-learn#12555)
thoo pushed a commit to thoo/scikit-learn that referenced this pull request Nov 13, 2018
thoo added a commit to thoo/scikit-learn that referenced this pull request Nov 13, 2018
…ikit-learn into add_codeblock_copybutton

* 'add_codeblock_copybutton' of https://github.com/thoo/scikit-learn:
  Move an extension under sphinx_copybutton/
  Move css/js file under sphinxext/
  Fix max_depth overshoot in BFS expansion of trees (scikit-learn#12344)
  TST don't test utils.fixes docstrings (scikit-learn#12576)
  DOC Fix typo (scikit-learn#12563<
8000
/a>)
  FIX Workaround limitation of cloudpickle under PyPy (scikit-learn#12566)
  MNT bare asserts (scikit-learn#12571)
  FIX incorrect error when OneHotEncoder.transform called prior to fit (scikit-learn#12443)
  Retrigger travis:max time limit error
  DOC: Clarify `cv` parameter description in `GridSearchCV` (scikit-learn#12495)
  FIX remove FutureWarning in _object_dtype_isnan and add test (scikit-learn#12567)
  DOC Add 's' to "correspond" in docs for Hamming Loss. (scikit-learn#12565)
  EXA Fix comment in plot-iris-logistic example (scikit-learn#12564)
  FIX stop words validation in text vectorizers with custom preprocessors / tokenizers (scikit-learn#12393)
  DOC Add skorch to related projects (scikit-learn#12561)
  MNT Don't change self.n_values in OneHotEncoder.fit (scikit-learn#12286)
  MNT Remove unused assert_true imports (scikit-learn#12560)
  TST autoreplace assert_true(...==...) with plain assert (scikit-learn#12547)
  DOC: add a testimonial from JP Morgan (scikit-learn#12555)
thoo pushed a commit to thoo/scikit-learn that referenced this pull request Nov 14, 2018
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Nov 14, 2018
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Nov 14, 2018
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0