Skip to content

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

Merged
merged 19 commits into from
Jun 10, 2025

Conversation

supersom
Copy link
Contributor

Reference Issues/PRs

Towards #30621

What does this implement/fix? Explain your changes.

  • Added example comparing L1-based models to ARD user guide
  • Integrated (existing) example comparing Bayesian Regressors into text

Any other comments?

If integration into text is the preferred model, should I remove the 'example' section for ARD?

supersom added 2 commits May 25, 2025 15:56
…ted example comparing Bayesian Regressors into text
…egrated example comparing Bayesian Regressors into text
Copy link

github-actions bot commented May 25, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 808a5dd. Link to the linter CI: here

Copy link
Contributor

@StefanieSenger StefanieSenger left a 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?

@supersom
Copy link
Contributor Author

Thanks for clarifying, @StefanieSenger!
I have removed the examples section in doc/modules/linear_model.rst and added the changes from #30587.

Is there something from your perspective that, were one of the two links need to be linked?

I'm not sure I followed you on this one - could you help me out please?

@StefanieSenger
Copy link
Contributor

Thank you for your further work, @supersom.

I'm not sure I followed you on this one - could you help me out please?

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):
- GL03: Double line break found; please use only one blank line to separate sections or paragraphs, and do not leave blank lines at the end of docstrings

Could you please fix these?

@supersom
Copy link
Contributor Author

Thanks for the review, @StefanieSenger!

I meant that I don't think the links from the other PR are really necessary, and asked for your opinion.

Crystal clear now - thank you!
I think these example links in the API docs that @marouaneaka had intended to place are quite illustrative.I have also added links to the user guide. I'm of the opinion that we should make examples discoverable from multiple independent locations - but I'll defer to your experience and oversight on whether that is the appropriate design call here.

GL03: Double line break found ... Could you please fix these?

Sure - I'm on it. New to reST and figuring out local testing - so it's taking a few turns.

Copy link
Contributor

@StefanieSenger StefanieSenger left a 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.

Comment on lines 846 to 849
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.)

Copy link
Contributor Author

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.

Copy link
Contributor

@StefanieSenger StefanieSenger left a 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?

@supersom
Copy link
Contributor Author

supersom commented Jun 3, 2025

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.

@adrinjalali adrinjalali merged commit 3962c28 into scikit-learn:main Jun 10, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants