Skip to content

DOC Cleaning parameter spec in docstrings for ensemble m…#16330

Merged
rth merged 4 commits intoscikit-learn:masterfrom
lopusz:doc_module_ensemble
Feb 2, 2020
Merged

DOC Cleaning parameter spec in docstrings for ensemble m…#16330
rth merged 4 commits intoscikit-learn:masterfrom
lopusz:doc_module_ensemble

Conversation

@lopusz
Copy link
Copy Markdown
Contributor

@lopusz lopusz commented Jan 31, 2020

Reference Issues/PRs

#15761

What does this implement/fix? Explain your changes.

Cleans up parameter spec in ensemble, so it follows the convention outlined in the Contributing Guide

Only changes in docstrings.

@lopusz lopusz changed the title DOC Cleaning defaults in parameter docstrings for ensemble module DOC Cleaning parameter spec in docstrings for ensemble module Jan 31, 2020
@lopusz lopusz force-pushed the doc_module_ensemble branch from 13aa55f to b3d850d Compare February 1, 2020 16:15
@lopusz lopusz force-pushed the doc_module_ensemble branch from b3d850d to 0b9d966 Compare February 1, 2020 16:55
@lopusz lopusz changed the title DOC Cleaning parameter spec in docstrings for ensemble module [MRG] DOC Cleaning parameter spec in docstrings for ensemble module Feb 1, 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.

LGTM, thanks!

- If "log2", then `max_features=log2(n_features)`.
- If 'auto', then `max_features=sqrt(n_features)`.
- If 'sqrt', then `max_features=sqrt(n_features)`.
- If 'log2', then `max_features=log2(n_features)`.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are these changes necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, perhaps I am overzealous here.

The story of this change was so:

First, I changed the max_features type from str to the set of allowed parameters. The contributing guide uses single ticks, so the default set was {'auto', 'sqrt', 'log2'}.

Second, I changed the parameter description for consistency to single ticks.

Shall I revert this? Just let me know.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The contributing guide uses single ticks, so the default set was {'auto', 'sqrt', 'log2'}.

I'm not sure there is a reason for it though. For instance, black will format with double tick in code. We can keep it as is here, but best to avoid such changes in future PRs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rth I will, thanks for pointing this out!

Does sciki-learn use black for formatting code (even optionally)?

I am a great fan of the tool, so if this was the case I would definitely blackify my further contribution (if any).

@lopusz
Copy link
Copy Markdown
Contributor Author

lopusz commented Feb 2, 2020

@rth, great!

Reviewing this patches is tedious. I appreciate it. I believe it makes the amazing documentation of scikit-learn even a tiny bit more amazing 😄

Thank you!

@rth
Copy link
Copy Markdown
Member

rth commented Feb 2, 2020

Thanks for working on this @lopusz !

@rth rth changed the title [MRG] DOC Cleaning parameter spec in docstrings for ensemble module DOC Cleaning parameter spec in docstrings for ensemble m… Feb 2, 2020
@rth rth merged commit f2146dd into scikit-learn:master Feb 2, 2020
@lopusz lopusz deleted the doc_module_ensemble branch February 2, 2020 20:37
thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 22, 2020
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020
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.

2 participants