-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DOC improve docs in DecisionBoundaryDisplay and linear models #30729
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
Conversation
@@ -248,14 +248,12 @@ def from_estimator( | |||
:func:`contour <matplotlib.pyplot.contour>`, | |||
:func:`pcolormesh <matplotlib.pyplot.pcolormesh>`. | |||
|
|||
response_method : {'auto', 'predict_proba', 'decision_function', \ | |||
response_method : {'auto', 'decision_function', 'predict_proba', \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioning the options for response_method
in the same order as they are checked through to reduce mental load for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fix.
I find it weird that we don't attempt to use predict_proba
first when available.
Maybe we could open a dedicated follow-up PR if people agree this would be a better default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that predict_proba
is less custom and easier to interpret than decision_function
, so yes if users use the default auto
they should maybe better see the more general info. Also, it would be good to add a title to the plot informing the user which response_method
was used.
doc/modules/linear_model.rst
Outdated
:class:`LinearRegression` will take in its ``fit`` method arguments ``X``, ``y``, | ||
``sample_weight`` and will store the coefficients :math:`w` of the linear model in its | ||
``coef_`` and ``intercept_`` attributes:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioning intercept_
attribute to avoid misunderstanding as if LinearRegression fits intercept by default.
I also substituted arrays
with arguments
, since the input can be array-likes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also drop the "will" and instead use the present tense while we are at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made it present tense.
There seems to be a floating point rounding issue with some CIs expecting Otherwise, we could change the doc example into
maybe? |
I have solved it by rounding. Please let me know if I should do it another way. |
doc/modules/linear_model.rst
Outdated
:class:`LinearRegression` will take in its ``fit`` method arguments ``X``, ``y``, | ||
``sample_weight`` and will store the coefficients :math:`w` of the linear model in its | ||
``coef_`` and ``intercept_`` attributes:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also drop the "will" and instead use the present tense while we are at it.
@@ -248,14 +248,12 @@ def from_estimator( | |||
:func:`contour <matplotlib.pyplot.contour>`, | |||
:func:`pcolormesh <matplotlib.pyplot.pcolormesh>`. | |||
|
|||
response_method : {'auto', 'predict_proba', 'decision_function', \ | |||
response_method : {'auto', 'decision_function', 'predict_proba', \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fix.
I find it weird that we don't attempt to use predict_proba
first when available.
Maybe we could open a dedicated follow-up PR if people agree this would be a better default.
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Merged because it looked good, not to end the discussion :) |
Hi @StefanieSenger, I get this on my mac:
What do you think? It's not blocking me in any way, it's just FYI.. or if I should do something about it (open an issue or so). Thanks |
Hi @DeaMariaLeon, thanks for bringing this up. I also get this locally with I would think this might be due to differences between by local doctest setup and the CI doctest setup, but not sure to be honest. A lot of other doctest things are also failling for me because of different rounding. It would be nice to learn how to mimic the CI's doctest locally, because it could save time in the future. |
This PR proposes doc improvements in the docstring of
DecisionBoundaryDisplay.from_estimator
and in andlinear_models.rst
to facilitate readability.