Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

DOC Add dropdowns in Module 3.3 #28355

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 9 commits into from
Feb 20, 2024

Conversation

manuel-morales-a
Copy link
Contributor

Reference Issues/PRs

Towards #26617.

What does this implement/fix? Explain your changes.

This PR implements the dropdowns in section 3.3, using the resources of #26625. The only file that had to been modified is model_evaluation.rst.

Thank you!

Any other comments?

Copy link

github-actions bot commented Feb 2, 2024

✔️ Linting Passed

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

Generated for commit: d8941c1. Link to the linter CI: here

Copy link
Member

@ArturoAmorQ ArturoAmorQ 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 the PR @manuel-morales-a! Now that you're working on this section, can you also please hide

Comment on lines 2358 to 2367
|details-start|
**Square root of the MSE**
|details-split|

Taking the square root of the MSE, called the root mean squared error (RMSE), is another
common metric that provides a measure in the same units as the target variable. RSME is
available through the :func:`root_mean_squared_error` function.

|details-end|

Copy link
Member

Choose a reason for hiding this comment

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

This paragraph is short enough not to be hidden in a dropdown. It is also a common use metric, so we rather keep it visible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good! It has been de-dropdowned in one of the following commits.

@manuel-morales-a
Copy link
Contributor Author

Thanks for the feedback @ArturoAmorQ! I've implemented your comments.

I have one remaining question. In 3.3.2.19. Class likelihood ratios, with the dropdowns I've implemented, the Examples section ends up surrounded by dropdowns (between Mathematical divergences and References). Should I move it above the Interpretation across varying prevalence dropdown, next to the main text, or simply leave it where it currently is?

Thank you!

@ArturoAmorQ
Copy link
Member

Should I move it above the Interpretation across varying prevalence dropdown, next to the main text, or simply leave it where it currently is?

I would rather move it above the Interpretation across varying prevalence dropdown, please :)

@manuel-morales-a
Copy link
Contributor Author

Thank you! It should be good to go now: moved the examples as discussed. Cheers

Comment on lines 583 to 585
|details-start|
**References**
|details-split|
Copy link
Member

Choose a reason for hiding this comment

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

Sorry just a last comment, I just noticed some of the references (those with the format [authorDATE]) are called in some other parts of this page, but as of today, dropdowns break the sphynx cross-referencing system. Can you please revert the reference back to a topic directive whenever at least one of the references are called elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem! I've reinstated the topic directives when needed in the commit below. I had to remove a couple of dropdowns but most remained

|details-start|
**One-vs-rest Algorithm**
|details-split|
.. topic:: One-vs-rest Algorithm:
Copy link
Member

Choose a reason for hiding this comment

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

I am so sorry! I may have not been clear enough 😅

The references get broken whenever they point inside a dropdown. Cross-references from the dropdown to the outside (such as this one) are perfectly fine!

The underlying issue is that the dropdown has to be unfolded for mozilla to access the reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No probs! I've updated the references to topic directives when required (assuming I understood correctly this time. If not, no problem and we'll keep modifying the rst)

Copy link
Member

@ArturoAmorQ ArturoAmorQ left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks again for your work and patience and congrats on your first scikit-learn merge :) Hope this is just the first one from a series of contributions!

@ArturoAmorQ ArturoAmorQ merged commit 5fe3417 into scikit-learn:main Feb 20, 2024
@ArturoAmorQ ArturoAmorQ changed the title Dropdowns in documentation, section 3.3 DOC Add dropdowns in Module 3.3 Feb 20, 2024
@manuel-morales-a
Copy link
Contributor Author

Thanks for the helpful tips and comments, Arturo! Talk to you soon in another contribution

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.

2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.