Hello @cgsavard, I would like to work on this. Can I start looking at the AgglomerativeClustering class?

@vachanda Go for it! We can continue posting here which ones we work on so that others know.

Thanks for coordinating this @cgsavard

Note to contributors: please follow the guidelines under: https://scikit-learn.org/stable/developers/contributing.html#guidelines-for-writing-documentation

@cgsavard, Is there a list of classes that have discrepancies or do we have to go through each of them and update them?

@vachanda I do not have a list, unfortunately. I have just been going through the files and seeing what needs to be updated.

I am working on AffinityPropagation, SpectralCoclustering, SpectralBiclustering, and Birch.

I am working on FeatureAgglomeration, KMeans, and MiniBatchKMeans.

Logically speaking, if a param is optional, shouldn't the default be None always? Having a parameter with a default value other than None suggests it should be required.

If there is a default, this usually means that the literature has found this to be a sensible default value which also suggests that this parameter has an impact on performance and thus it shouldn't be optional, but should just mention what the default is. Those seem closer to required parameters by definition, we just happened to make a sensible choice for the user so they can change it or not.

Or more practically speaking, are there currently any optional parameters that we've found which have numeric default values, but for which specifying None will raise an exception? That would also suggest that the parameter is actually required, but that a sensible default has been chosen based on literature/research.

Or maybe I've been confusing the meaning of required and optional all these years? Lol. Would definitely love to help on this either way!

@jmwoloso We were really inconsistent regarding the usage of optional and therefore we recently decided to remove it.

i want to contribute as well. can i go ahead with this

@glemaitre ok, that definitely makes sense. so then we're removing the optional verbage all together, right, while also noting default values in the doc strings?

should each of these that we find be opened as an issue separately or how are we staging all of this work that we're doing since multiple people are working on multiple things relating to this single issue?

@cyrus303 @jmwoloso You can get a class (a module maximum) and correct it. The idea is to remove the optional and add a default when there is one (there is usually one). Since we are touching the documentation, we should make sure that the style on the line follow our new style guide: https://scikit-learn.org/dev/developers/contributing.html#guidelines-for-writing-documentation

You can mention which of the class/module you are working, open a link a PR to avoid duplicate effort :). Looking forward to reviewing it.

Hey! I will work on tree classes (tree.DecisionTreeClassifier, tree.DecisionTreeRegressor, tree.ExtraTreeClassifier and tree.ExtraTreeRegressor).

I will also fix this issue for the neighbors module.

I'll take the ensemble module.

@glemaitre any preference on bool vs. boolean? seeing a mix of both in ensemble, even in the same class. might as well get those in shape while i'm doing defaults.

EDIT:

ditto for int vs integer. I'm assuming int on that one, but wanted to confirm.

EDIT (again):

also seeing docstrings with inconsistent values relative to the __init__ signature for that class, e.g.:

min_impurity_split for RandomForestClassifier

the __init__ signature has min_impurity_split=None while the docstrings for it say min_impurity_split : float, (default=0). I would assume update the docstrings to match the signature since we'd want to keep the behavior of the class consistent (i.e. we want the same defaults passed in upon instantiation)?

@jmwoloso Could you refer to https://scikit-learn.org/stable/developers/contributing.html#guidelines-for-writing-documentation. Basically you should default to the python type name (bool, str, int, float)

the init signature has min_impurity_split=None while the docstrings for it say min_impurity_split : float, (default=0). I would assume update the docstrings to match the signature since we'd want to keep the behavior of the class consistent (i.e. we want the same defaults passed in upon instantiation)?

We should match the parameter in the function signature. This value default parameter has changed and the docstring was not updated.

Hi @cgsavard , I'd like to contribute but this is going to be my first time so need some hand holding. I'm quite familiar with python, somewhat handy with text editors and recently went through the fork -> clone -> edit -> PR workflow tutorial here. Please advise next step... Thank you!

Hi @cgsavard ,
Can I please work on Imputer ?

Hi, I am new, and I would like to start contributing. Do you still need some help on this issue? is there any file you still need help with?

Hey @boricles!

Have a look to #15761 (comment) for a list with the modules still to be fixed.

@alfaro96 thanks. I did a quick look just now. I will select a module tonight, and work on it.

I am working on sklearn/config_context

Hey, thought I'd see if I could help with the docs.

@alfaro96 I'd like to work on sklearn.feature_extraction.text.CountVectorizer, if it hasn't already been taken, especially because I've personally encountered some pitfalls when working with Vectorizers in the past.

Also, I noticed that although sklearn.model_selection.learning_curve was updated, there's an out-of-date tutorial using the old documentation, should I leave it be? Or is it worth updating?

Hi @alfaro96,

after edits:
I see sklearn.config_context and sklearn.set_config from sklearn.config_config.py were fixed so it can be checked out from the task list.

I would like to work on sklearn.utils. I saw only once instance of parameter documentation where 'optional' is used in. That means that I need to fix only that instance, correct? It is in sklearn.utils._mocking.py

Hey, thought I'd see if I could help with the docs.

Hey @madprogramer,

@alfaro96 I'd like to work on sklearn.feature_extraction.text.CountVectorizer, if it hasn't already been taken, especially because I've personally encountered some pitfalls when working with Vectorizers in the past.

I have had a look to the checklist and sklearn.feature_extraction.text.CountVectorizer reference and it does not seem to be fixed. A PR would be welcome.

Edit: The sklearn.feature_extraction.text.CountVectorizer is already fixed.

Also, I noticed that although sklearn.model_selection.learning_curve was updated, there's an out-of-date tutorial using the old documentation, should I leave it be? Or is it worth updating?

It is worth it updating, although this should be done in a separate PR.

Thank you!

Hi @alfaro96,

Hey @haiatn,

after edits:
I see sklearn.config_context and sklearn.set_config from sklearn.config_config.py were fixed so it can be checked out from the task list.

I have updated the checklist.

I would like to work on sklearn.utils. I saw only once instance of parameter documentation where 'optional' is used in. That means that I need to fix only that instance, correct? It is in sklearn.utils._mocking.py

That is the idea, although the classes in the sklearn.utils._mocking.py file are not part of the public API, so I do not think that it is worthy to update them.

Nevertheless, it would be nice if you could work in any of the other functions, classes and modules that are pending to be fixed.

Thank you!

I looked at the checklist. From what I saw the following can be checked from the checklist:

• sklearn.feature_extraction.image.img_to_graph
• sklearn.isotonic.IsotonicRegression
• sklearn.isotonic.check_increasing
• I did not find the file sklearn.ensemble.HistGradientBoostingRegressor but all of sklearn.ensemble is OK

Can I work on sklearn.manifold._spectral_embedding and sklearn.feature_extraction.text.HashVectorizer? I will do it in seperate PR. I think they are the only files left that need fixing (assuming sklearn.feature_extraction.text.CountVectorizer is taken).

I looked at the checklist. From what I saw the following can be checked from the checklist:

• sklearn.feature_extraction.image.img_to_graph
• sklearn.isotonic.IsotonicRegression
• sklearn.isotonic.check_increasing

Thank you @haiatn, I have updated the checklist.

• I did not find the file sklearn.ensemble.HistGradientBoostingRegressor but all of sklearn.ensemble is OK

The sklearn.ensemble.HistGradientBoostingClassifier and sklearn.ensemble.HistGradientBoostingRegressor are in this file: scikit-learn/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py. However, they have been already fixed.

Can I work on sklearn.manifold._spectral_embedding and sklearn.feature_extraction.text.HashVectorizer? I will do it in seperate PR. I think they are the only files left that need fixing (assuming sklearn.feature_extraction.text.CountVectorizer is taken).

I have had a look to the sklearn.manifold module and sklearn.feature_extraction.text.HashingVectorizer and they have been already fixed (I have updated the checklist accordingly).

Nevertheless, there are several functions in the sklearn.utils module that should be still fixed.

Thank you @haiatn, we really appreciate your help!

I will now work on sklearn.utils._estimator_html_repr, sklearn.utils.deprecation and sklearn.utils._testing

I will finish sklearn.utils. There are only 3 files I found that need fixing.

hey @alfaro96 ,
could you review my open pull requests? I think they are the last ones.
#18360 #18385 #18386

Hey @haiatn!

I have already have a look to your open PRs.

Thank you!

Now that we merged what's left off sklearn.utils and it was the last on the checklist, did we finish?

There is one last open pull request #18025, then this issue could be eventually closed.

Hello,
I want to start contributing. Is there any class pending for fixing doc of default values? If any then I can take it up.