Skip to content

DOC [PST] FIX/RFC machine learning map #28630

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

Conversation

Charlie-XIAO
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO commented Mar 14, 2024

Please note that this PR targets the new_web_theme branch!

Towards #28084. Check the rendered docs here.

Prior to this PR the machine learning map is a PNG image and the links on the nodes are realized using the <map> component. However the <map> requires hard-coded positions and will not automatically scale as the width and height of the image changes. The only way is to use transform: scale(xxx). The problem with this is: transform does not change width and height so the image will still take up that much space on the page. The solution prior to this PR was to use position: absolute but this will break pydata-sphinx-theme.

The solution of this PR is to use SVG+XML, which natively allows links so there is no need for <map> and no problem of image scaling. The chart is created using draw.io, and to modify the chart one simplify import the chart into draw.io, make changes, and re-export.

This PR also supports zoom on wheel and pan on drag when JavaScript is available using svg-pan-zoom-container. It also supports dark mode by inverting the colors and rotating the hues.

I've tried my best to simulate the original chart. The nodes and their layout are also the same, but I don't know how to create irregular shapes in draw.io so I used the rounded rectangles for the four main blocks instead.

Choosing.the.right.estimator.scikit-learn.1.5.dev0.documentation.9.-.-.Microsoft.Edge.2024-03-15.02-44-49.mp4

Copy link

github-actions bot commented Mar 14, 2024

✔️ Linting Passed

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

Generated for commit: 8a4dd0d. Link to the linter CI: here

@Charlie-XIAO Charlie-XIAO marked this pull request as ready for review March 14, 2024 18:49
Copy link
Member

@thomasjpfan thomasjpfan 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! It looks like we can pan all the way to the right and have the map go off screen. Is this expected behavior?

Screen Recording 2024-03-14 at 2 56 28 PM

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Mar 14, 2024

This is not desired. Sadly I think svg-pan-zoom-container does not support setting boundaries. I'm not sure if there are other packages that serve this purpose?

Edit: I think I've figured out a way to limit the pan. I will update when I get some time to test.

@Charlie-XIAO
Copy link
Contributor Author

Okay so this new version limits the pan. It relies on svg-pan-zoom which is more flexible, and you may look at its demos to see if there are other desired functionalities that we may want to add here.

Choosing.the.right.estimator.scikit-learn.1.5.dev0.documentation.8.-.-.Microsoft.Edge.2024-03-15.20-38-20.mp4

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

LGTM. I'll let @thomasjpfan or @ogrisel have another look.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Overall looks good! I left a comment about including the license of the vendoered svg-pan-zoom.

@Charlie-XIAO
Copy link
Contributor Author

License added!

@thomasjpfan thomasjpfan merged commit 321d683 into scikit-learn:new_web_theme Apr 30, 2024
21 checks passed
@Charlie-XIAO Charlie-XIAO deleted the pst-estimator-map branch August 13, 2024 07:42
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