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

Updated _output_2d_ attributes documentation for DummyClassifier and… #14801

Closed

Conversation

nitya
Copy link

@nitya nitya commented Aug 24, 2019

… DummyRegressor

Reference Issues/PRs

Partially addresses #14312

What does this implement/fix? Explain your changes.

Added documentation for output_2d attribute in DummyRegressor and DummyClassifier

Any other comments?

@NicolasHug
Copy link
Member

Thanks for the PR @nitya

I'm pretty sure that we always have

self.output_2d_ == (self.n_outputs_ != 1)

and that output_2d_ is redundant. I'd be happy to see it deprecated and removed. Do you want to give it a try @nitya ?

@nitya
Copy link
Author

nitya commented Aug 27, 2019

Will give it a try tomorrow. Thank you!

@nitya
Copy link
Author

nitya commented Aug 28, 2019

@NicolasHug
Looking for clarifications here:

(Line 123) - self.output_2d_ is defined as:
self.output_2d_ = y.ndim == 2 and y.shape[1] > 1

(Line 125-128) - subsequently, self.n_outputs_ is defined:

if y.ndim == 1:
     y = np.reshape(y, (-1, 1))

self.n_outputs_ = y.shape[1]

My original PR was for the docstring (documentation of attribute) for output_2d_ and not for any code changes.

So clarification requested:

  1. Are you suggesting I remove the self.output_2d_ attribute completely and just use (self.n_outputs_ != 1) as the inline replacement? OR

  2. Are you suggesting we keep the attribute but simply update its definition to this simple check vs. the ones above?

If #1, then need to make only code changes (original docstring-related PR is irrelevant)
If #2, then I can make that change but can you verify the docstring PR (language used) is correct?

@NicolasHug
Copy link
Member

I'm suggesting that we deprecate the attribute, meaning that you would have to:

  • use n_outputs instead (like you mentioned in 1.)
  • create a property that raises a deprecation warning when outputs_2d_ is used.

You can look at our deprecation guidelines in the contributing guide.

@NicolasHug
Copy link
Member

Do you need any help @nitya ? Or should somelse take it up?

@NicolasHug
Copy link
Member

@nitya FYI I have opened a PR for deprecating the attribute #14933, which would close this one once/if merged

@reshamas
Copy link
Member

reshamas commented Sep 9, 2019

@kellycarmody
Can you drop Nitya an email? Thanks.

@kellycarmody
Copy link
Contributor

Thanks for your help @nitya! Really appreciate it!

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.

None yet

4 participants