Skip to content

Deprecate public attributes in linear_models SGD classes#16261

Merged
glemaitre merged 20 commits intoscikit-learn:masterfrom
chbrandt:issue_14364
Feb 12, 2020
Merged

Deprecate public attributes in linear_models SGD classes#16261
glemaitre merged 20 commits intoscikit-learn:masterfrom
chbrandt:issue_14364

Conversation

@chbrandt
Copy link
Copy Markdown
Contributor

@chbrandt chbrandt commented Jan 28, 2020

Reference Issues/PRs

Partially addresses #14364

What does this implement/fix? Explain your changes.

Attributes were made "private", properties with the deprecated names were added as indicated in the docs

@cmarmo
Copy link
Copy Markdown
Contributor

cmarmo commented Jan 28, 2020

Thanks @chbrandt ! Could you please fix the lint errors?

Copy link
Copy Markdown
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

Thanks @chbrandt. Here are a few comments. Please also add a what's new entry in v0.23.rst.

@rth
Copy link
Copy Markdown
Member

rth commented Jan 29, 2020

Thanks. Please rename this PR with a more explicit title. There is no need to mention the issue ID in the title since most people seeing it in the list will not know what it is.

@chbrandt chbrandt changed the title Fix issue #14364 Deprecate public attributes in linear_models SGD classes Jan 29, 2020
Copy link
Copy Markdown
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

I just still have one remark about new attribute names.

rth
rth previously requested changes Jan 29, 2020
Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

(Just changing status to "changes requested", to indicate that no new reviews are needed at this point)

Copy link
Copy Markdown
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

Here's a new batch of comments

Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

LGTM, aside for two comments below. Thanks @chbrandt !

@rth rth dismissed their stale review February 5, 2020 13:30

Previous comments were addressed.

Copy link
Copy Markdown
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

looks good @chbrandt. Just a few remaining comments

Copy link
Copy Markdown
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

I pushed some nitpicks.
lgtm, thanks @chbrandt !

ping @glemaitre for another review

Copy link
Copy Markdown
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM. Waiting for CIs to turn green before merging.

@glemaitre glemaitre merged commit 4913037 into scikit-learn:master Feb 12, 2020
@glemaitre
Copy link
Copy Markdown
Member

I am merging. The issue with the CI is due to we did not merge master into the branch. This should be safe.

Thanks @chbrandt :)

@chbrandt
Copy link
Copy Markdown
Contributor Author

Cool! Thanks guys :)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants