Skip to content

Rewriting of the example code (plot_lle_digits.py) to generate again the broken images#22165

Closed
cathelineparisot wants to merge 5 commits intoscikit-learn:mainfrom
cathelineparisot:#22061fix]-image-documentation
Closed

Rewriting of the example code (plot_lle_digits.py) to generate again the broken images#22165
cathelineparisot wants to merge 5 commits intoscikit-learn:mainfrom
cathelineparisot:#22061fix]-image-documentation

Conversation

@cathelineparisot
Copy link
Copy Markdown

Fixes #22061

What does this implement/fix? Explain your changes.

The image in the MDS document is not present
https://scikit-learn.org/stable/modules/manifold.html#multi-dimensional-scaling-mds

I changed the path of the image to something similar to the old version: https://scikit-learn.org/0.24/modules/manifold.html#multi-dimensional-scaling-mds

Any other comments?

Other images are not present in this same documentation page: only sphx_glr_plot_lle_digits_001.png and sphx_glr_plot_lle_digits_002.png exist in the image folder (doc/auto_example/manifold/images) but all others do not (3,4,5,6,7,8,9 and 13). This could be the occasion of another issue. I don't know what images to put instead. For my part, I changed the image 010 to 002.

Mine was generated from https://scikit-learn.org/dev/auto_examples/manifold/plot_lle_digits.html#sphx-glr-auto-examples-manifold-plot-lle-digits-py

@cathelineparisot
Copy link
Copy Markdown
Author

Excuse me for all these commits, I got confused because I had made commits on the main directly for another issue. I only changed the path of the image for this issue.

@cathelineparisot cathelineparisot changed the title #22061fix] image documentation #22061[fix] image documentation Jan 10, 2022
@cathelineparisot cathelineparisot changed the title #22061[fix] image documentation #22061[fix]fix-image-documentation Jan 10, 2022
Copy link
Copy Markdown
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.

Thank you for the PR @cathelineparisot

space and the similarities/dissimilarities.

.. figure:: ../auto_examples/manifold/images/sphx_glr_plot_lle_digits_010.png
.. figure:: ../auto_examples/manifold/images/sphx_glr_plot_lle_digits_002.png
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do not think this is enough to fix mainfold.rst because of all the missing images. I think we need to rework #20543 so that all the images are actually generated.

Basically #20543 lead to a regression in mainfold.rst. The previous version was better: https://scikit-learn.org/0.24/modules/manifold.html#multi-dimensional-scaling-mds

"or be the string 'passthrough' "
"'%s' (type %s) doesn't" % (t, type(t))
)
if (not (hasattr(t, "fit") or hasattr(t, "fit_transform")) or not
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is overlapping with another PR at #22162, we can focus on that there.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I didn't do it on purpose, I handled Git wrong but I put back the original code.

@cathelineparisot
Copy link
Copy Markdown
Author

cathelineparisot commented Jan 12, 2022

@thomasjpfan

Thank you very much for your first feedback. I didn't understand what the problem was and now I've made the connection.
To fix this, I made now a mix between the new version of plot_lle_digits.py and the old version present in #20543.
I kept all the function grouping as it was done in the new version but I took the old code for plot_embedding and tried to bring some aspects of the new code like the axis.

Does it look right to you?

The images are well generated again :)

Also, I have this error:

WARNING: Since version 2.0, Sphinx uses "index" as default root_doc. Please add "root_doc = 'contents'" to your conf.py.

Should we add this to the Sphinx config?

Thanks in advance for your feedback

@cathelineparisot cathelineparisot force-pushed the #22061fix]-image-documentation branch from e18a300 to 724f719 Compare January 12, 2022 22:25
@cathelineparisot cathelineparisot changed the title #22061[fix]fix-image-documentation Rewriting of the example code (plot_lle_digits.py) to generate again the broken images Jan 12, 2022
@thomasjpfan
Copy link
Copy Markdown
Member

thomasjpfan commented Jan 12, 2022

Should we add this to the Sphinx config?

No it is unrelated to the PR. If we want to update sphinx config, it should be done in another PR.

There are a few things that needs to be done with this PR:

  1. The Azure CI is failing because of linting issues having to do with black. You can find instructions on how to run black in the Pull Request checklist on step 5. (Install black==21.6b0 and run black .) Once the linting issues are fixed circleci will render this PR's docs, so we can see if the HTML are rendered correctly.
  2. We have to make sure that both https://scikit-learn.org/stable/auto_examples/manifold/plot_lle_digits.html and mainfold.rst are generated nicely in HTML
  3. The best workflow to do part 2 is described in Building the documentation where you can use EXAMPLES_PATTERN to build one example locally.
  4. To rework plot_lle_digits.py one needs to be aware of the narrative style docs we are moving toward. For example: https://github.com/scikit-learn/scikit-learn/blob/main/examples/compose/plot_column_transformer_mixed_types.py there is a mixture of using # %% to construct cells so the rendered HTML has separate headings.

With all the points above, I do not think this is a great first issue. I recommend looking at #21350 for a good first issue to start getting use to scikit-learn's workflow.

@thomasjpfan
Copy link
Copy Markdown
Member

@cathelineparisot Would you be okay if another contributor continues this work?

@cathelineparisot
Copy link
Copy Markdown
Author

@cathelineparisot Would you be okay if another contributor continues this work?

Hello, yes of course. I am no longer working on this issue so someone can take over. Thanks for asking !

@cmarmo cmarmo added the Superseded PR has been replace by a newer PR label Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module:pipeline Superseded PR has been replace by a newer PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Broken image in MDS user guide documentation

3 participants