DOC List estimators that supports missing data in user guide #23198

MaxwellLZH · 2022-04-23T08:36:35Z

Fixes #21382. This is based on the work of lisekleiber

What does this implement/fix? Explain your changes.

Code refactoring and adding formatting as suggested in the original PR #22096

MaxwellLZH · 2022-04-23T08:57:50Z

The styling of the documentation is still not working as expected, I wasn't able to figure out where's the issue, could really need some help here

cmarmo · 2022-04-23T09:22:01Z

Hi @MaxwellLZH, thanks for your follow-up.
About the rendering, I would suggest to create a bullet list here.

List of estimators that allow NaN values for type regressor:
- :class:sklearn.ensemble._hist_gradient_boosting.gradient_boosting.HistGradientBoostingRegressor
List of estimators that allow NaN values for type classifier:
- :class:sklearn.ensemble._hist_gradient_boosting.gradient_boosting.HistGradientBoostingClassifier
etc...

Also, do you mind editing the description with "Fixes" or "Solves" and the issue number just after? This will link the issue to the PR and close the issue once the PR is merged. Thanks!

MaxwellLZH · 2022-04-23T09:45:05Z

HI @cmarmo, I've edited the description as suggested.

I'm kind of stuck with the formatting here, I checked the output string (as shown below), which seems to have the right format, but it's still shown as plain text rather than markdown in the generated HTML :(, would you mind help take a look at the code here, thank you!

* List of estimators that allow NaN values for type *regressor*:
 * :class:`sklearn.ensemble._hist_gradient_boosting.gradient_boosting.HistGradientBoostingRegressor`

cmarmo · 2022-04-24T04:30:30Z

Hi @MaxwellLZH I'm not an expert in sphinx and docutils.... so, please, bear with me.
My understanding is that the the output text is written in a docutils paragraph node, translated as an html <p> tag.
So I guess you can write the paragraph in HTML not rst.... this will possibly work but it is not a clean solution....
A clean solution would probably be the use of docutils bullet_list node. Do you think you can give it a try?

I still need to figure out how the links to the estimators are represented.... sorry...

MaxwellLZH · 2022-04-24T05:48:49Z

Hi @cmarmo, thank you for the instructions! I will take a look at using bullet_list node this week :)

MaxwellLZH · 2022-05-01T08:59:27Z

Sorry, I tried to figure out how to work with docutils.nodes.bullet_list and the link to classes with :class: but still wasn't able to fix the formatting :(

If someone familiar with sphinx and docutils could help move on with this PR or give some instructions on how to make it work would be really appreciated !

cmarmo · 2022-05-02T08:38:14Z

@MaxwellLZH I have opened a pull request to your branch.
The code is translated to a docutils node structure.
Could you please check the url to the estimators? Thanks!

Convert to node structure.

MaxwellLZH · 2022-05-02T11:31:57Z

Hi @cmarmo, thank you so much for helping! I've updated the estimator links and checks it's working now :)

ogrisel

Thank you all. This looks good to me!

thomasjpfan

Thank you for the PR!

doc/sphinxext/allow_nan.py

doc/modules/impute.rst

thomasjpfan · 2022-05-02T18:17:29Z

doc/sphinxext/allow_nan.py

+            try:
+                est = _construct_instance(est_class)
+            except:


I think this raises a SkipTest exception. In those cases, I think we can skip without warning, since warning is not that actionable here.

Suggested change

try:

est = _construct_instance(est_class)

except:

from contextlib import supress

...

with suppress(SkipTest):

est = _construct_instance(est_class)

I suspect the "Estimators that handle NaN values" section is for non meta-estimators anyways.

Make sense! I've updated code as suggested :)

Co-authored-by: Thomas J. Fan <[email protected]>

thomasjpfan

Minor comments, otherwise LGTM

doc/conf.py

thomasjpfan · 2022-05-03T20:45:59Z

doc/sphinxext/allow_nan.py

@@ -0,0 +1,59 @@
+from sklearn.utils import all_estimators


To be consistent with the class name, can you rename this: doc/sphinxext/allow_nan_estimators.py?

update doc/conf Co-authored-by: Thomas J. Fan <[email protected]>

thomasjpfan

LGTM

…learn#23198) Co-authored-by: Thomas J. Fan <[email protected]> Co-authored-by: Lise <[email protected]> Co-authored-by: Chiara Marmo <[email protected]>

Co-authored-by: Thomas J. Fan <[email protected]> Co-authored-by: Lise <[email protected]> Co-authored-by: Chiara Marmo <[email protected]>

…learn#23198) Co-authored-by: Thomas J. Fan <[email protected]> Co-authored-by: Lise <[email protected]> Co-authored-by: Chiara Marmo <[email protected]>

lisekleiber and others added 9 commits December 25, 2021 19:56

add doc section about estimators that handle NaN

34b1113

DOC update the error message introduced in scikit-learn#21219

bb46140

Merge remote-tracking branch 'upstream/main' into main

c5b2817

change length line error message

3a09f01

add changes in changelog

cc55635

reformat code allow_nan.py

fc5d267

ident error fixed

4173465

replace section id with link

5ef3df9

refactor code

074e3db

github-actions bot added module:utils Documentation labels Apr 23, 2022

MaxwellLZH added 3 commits April 23, 2022 16:38

merge main & fix conflict

99ac9f8

update pr number

891feee

minor changes

a01389d

format with bullet list

c9e9060

black formatting

41b1350

thomasjpfan mentioned this pull request Apr 24, 2022

DOC Maintain a list of all estimators that natively handle missing data #21382 #22096

Closed

merge main

8f1f32a

Convert in node structure.

fe91275

cmarmo mentioned this pull request May 2, 2022

Convert to node structure. MaxwellLZH/scikit-learn#1

Merged

MaxwellLZH added 2 commits May 2, 2022 17:57

Merge pull request #1 from cmarmo/estimator-handle-missing

663a428

Convert to node structure.

fix estimator class link & failed to construct warning

a19f831

MaxwellLZH added 2 commits May 2, 2022 20:59

apply black

2e5ea55

update whatsnew

bbc4247

ogrisel approved these changes May 2, 2022

View reviewed changes

thomasjpfan reviewed May 2, 2022

View reviewed changes

MaxwellLZH and others added 4 commits May 3, 2022 09:40

use python naming convention

e19a05b

Co-authored-by: Thomas J. Fan <[email protected]>

update directory name

a8d3aeb

Co-authored-by: Thomas J. Fan <[email protected]>

update directory name

774538a

Co-authored-by: Thomas J. Fan <[email protected]>

replace try except with suppress(SkipTest)

92512dd

thomasjpfan reviewed May 3, 2022

View reviewed changes

MaxwellLZH and others added 2 commits May 4, 2022 16:03

Update doc/conf.py

5c8bd21

update doc/conf Co-authored-by: Thomas J. Fan <[email protected]>

rename to allow_nan_estimators

070fd5e

thomasjpfan approved these changes May 4, 2022

View reviewed changes

thomasjpfan changed the title ~~DOC Maintain a list of all estimators that supports missing data~~ DOC List estimators that supports missing data in user guide May 4, 2022

thomasjpfan merged commit d3050e4 into scikit-learn:main May 4, 2022
28 checks passed

glemaitre pushed a commit that referenced this pull request May 19, 2022

DOC List estimators that supports missing data in user guide (#23198)

806090e

Co-authored-by: Thomas J. Fan <[email protected]> Co-authored-by: Lise <[email protected]> Co-authored-by: Chiara Marmo <[email protected]>

eddiebergman mentioned this pull request Nov 15, 2022

Update scikit learn 1.2 automl/auto-sklearn#1611

Open

54 tasks

May	APR	May
	27
2022	2023	2025

DOC List estimators that supports missing data in user guide #23198

DOC List estimators that supports missing data in user guide #23198

MaxwellLZH commented Apr 23, 2022 •

edited

MaxwellLZH commented Apr 23, 2022

cmarmo commented Apr 23, 2022

MaxwellLZH commented Apr 23, 2022

cmarmo commented Apr 24, 2022

MaxwellLZH commented Apr 24, 2022

MaxwellLZH commented May 1, 2022 •

edited

cmarmo commented May 2, 2022

MaxwellLZH commented May 2, 2022

ogrisel left a comment

thomasjpfan left a comment

thomasjpfan May 2, 2022

MaxwellLZH May 3, 2022

thomasjpfan left a comment

thomasjpfan May 3, 2022

MaxwellLZH May 4, 2022

thomasjpfan left a comment

DOC List estimators that supports missing data in user guide #23198

DOC List estimators that supports missing data in user guide #23198

Conversation

MaxwellLZH commented Apr 23, 2022 • edited

What does this implement/fix? Explain your changes.

MaxwellLZH commented Apr 23, 2022

cmarmo commented Apr 23, 2022

MaxwellLZH commented Apr 23, 2022

cmarmo commented Apr 24, 2022

MaxwellLZH commented Apr 24, 2022

MaxwellLZH commented May 1, 2022 • edited

cmarmo commented May 2, 2022

MaxwellLZH commented May 2, 2022

ogrisel left a comment

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan May 2, 2022

Choose a reason for hiding this comment

MaxwellLZH May 3, 2022

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan May 3, 2022

Choose a reason for hiding this comment

MaxwellLZH May 4, 2022

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

MaxwellLZH commented Apr 23, 2022 •

edited

MaxwellLZH commented May 1, 2022 •

edited