-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
DOC Added example comparing L1-based models to ARD user guide #31425
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
…ted example comparing Bayesian Regressors into text
…egrated example comparing Bayesian Regressors into text
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.
Thank you for taking this back up, @supersom!
If integration into text is the preferred model, should I remove the 'example' section for ARD?
Yes, you definitely should remove the examples section in doc/modules/linear_model.rst
(li. 845-849 currently), since this contains one of the two examples you have linked above and we don't want to have duplicated links.
I would like to finish on these two links, since it seems that #30587 is stalled.
Apart from this, sphx_glr_auto_examples_linear_model_plot_lasso_and_elasticnet.py
is already linked sufficiently in the user guide, I belive, and sphx_glr_auto_examples_linear_model_plot_ard.py
is already linked in ARDRegressor's docstring. Is there something from your perspective that, were one of the two links need to be linked?
…ARD docstrings and demonstrating ARD to ARD docstring [as in PR scikit-learn#30587]
…nto doc_link_example
Thanks for clarifying, @StefanieSenger!
I'm not sure I followed you on this one - could you help me out please? |
Thank you for your further work, @supersom.
Oh I see I did not express myself well, sorry about that. I meant that I don't think the links from the other PR are really necessary, and asked for your opinion. According to CI there are also several places with double line breaks (we are using numpydoc style guide): Could you please fix these? |
Thanks for the review, @StefanieSenger!
Crystal clear now - thank you!
Sure - I'm on it. New to reST and figuring out local testing - so it's taking a few turns. |
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 fixing the rendering issues, @supersom.
Now, two examples appear twice, and we need to remove the duplicates.
I am not so convinced about the links in the docstrings, since I think general examples like these are more a matter for the user guide, and we want to keep examples section in the API reference as clean as possible and they should be for pure code examples only.
But I will leave this for a maintainer to decide on.
doc/modules/linear_model.rst
Outdated
See :ref:`sphx_glr_auto_examples_linear_model_plot_ard.py` for a worked-out comparison between ARD and `Bayesian Ridge Regression`_. | ||
|
||
See :ref:`sphx_glr_auto_examples_linear_model_plot_lasso_and_elasticnet.py` for a comparison between various methods - Lasso, ARD and ElasticNet - on correlated data. | ||
|
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.
See :ref:`sphx_glr_auto_examples_linear_model_plot_ard.py` for a worked-out comparison between ARD and `Bayesian Ridge Regression`_. | |
See :ref:`sphx_glr_auto_examples_linear_model_plot_lasso_and_elasticnet.py` for a comparison between various methods - Lasso, ARD and ElasticNet - on correlated data. |
Now these two are linked twice. So, we should remove the duplicates. (The line after should also be removed, to avoid double blank lines again, but I cannot comment there.)
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.
Now these two are linked twice.
Thank you so much for pointing that out, @StefanieSenger - I overlooked auto-merge getting too helpful! Should be fixed now.
we want to keep examples section in the API reference as clean as possible and they should be for pure code examples only.
That sounds like a reasonable plan - code examples in docstrings and general examples in user guides. Since #30587 was already underway, I just brought those changes over TBH.
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 your most recent adjustments, @supersom.
Now everything is in good shape. Only the question on whether to add these examples to the API reference of ElasticNet
, Lasso
and ARDRegressor
is still open. What would you think, @adrinjalali?
Hi @adrinjalali , @StefanieSenger - Please let me know how we could take this to a merge. |
Reference Issues/PRs
Towards #30621
What does this implement/fix? Explain your changes.
Any other comments?
If integration into text is the preferred model, should I remove the 'example' section for ARD?