scikit-learn-contrib / metric-learn

Metric learning algorithms in Python
http://contrib.scikit-learn.org/metric-learn/
MIT License
1.4k stars 234 forks source link

[MRG][DOC] Fixes almost all warnings in the docs #338

Closed mvargas33 closed 3 years ago

mvargas33 commented 3 years ago

Closes #226 Closes #305

While doing the docs for #333 #329 and #330 I've encountered an enormous amount of warnings while executing make html. This PR solves most of these warnings while leaving a small number of warnings open to discussion. Here's a detail of the changes:

  1. Correct dependencies: In setup.py solved a typo in shinx_rtd_theme for sphinx_rtd_theme. Also added sphinx-galleryand matplotlib, both needed to render the docs for the examples.
  2. Fixed deprecated functions: The docs won't compile as they are. In conf.py, at line 82 and 83 add_stylesheet need to be changed for add_css_file, and add_javascript for add_js_file. Also html4_writer = True at line 72 is deprecated by an explicit warning from sphinx_rtd_theme , so it's commented now.
  3. Most warnings come from the any flag: As stated in #305 the any flag is causing more trouble than benefit. So I commented it. Tested some refs in the docs and they seem to work as usual.
  4. Topic directive without body: Each topic directive regarding ''Example Code'' had no body, mainly in supervised.rst and weakly_supervised.rst. Introduced a brief description of the code whenever necessary, like "Example Code: A basic usage of this model with the Iris dataset from scikit-learn."
  5. Minor issues: Some warnings were solved simply by using the right indentation.
  6. Last remaining warnings: In supervised.rst, weakly_supervised.rst and unsupervised.rst there is still an issue:

Some possible solutions:

Besides this last problem, all warnings were fixed.

bellet commented 3 years ago

Thanks a lot @mvargas33, this is a really useful contribution. I will take a look at this in details but it would be great if @wdevazelhes could also review it!

bellet commented 3 years ago

Also, the references and example environments are a bit ugly visually (just a simple border with possibly overlarge margins). Maybe there is a way to make this a bit more nice?

mvargas33 commented 3 years ago

Also, the references and example environments are a bit ugly visually (just a simple border with possibly overlarge margins). Maybe there is a way to make this a bit more nice?

I've changed the CSS of Example Code and References for a custom one. You can notice the differences here before and after this PR.

mvargas33 commented 3 years ago

Also, the references and example environments are a bit ugly visually (just a simple border with possibly overlarge margins). Maybe there is a way to make this a bit more nice?

I've changed the CSS of Example Code and References for a custom one. You can notice the differences here before and after this PR.

Forgot to mention that this approach don't use .. topic:: References but a simple dot list instead, finally removing all warnings from compilation.

Also, with this last commit, I've added all methods from all classes to their respective Methods directive. I've also checked that the methods appear in alphabetical order, just as they are rendered by autosummary.

So in the end, each class has a summary of all of its methods (in addition to the attributes and references and so on) and then the detailed description of each method.

I also added the autosummary for MetricTransformer and MahalanobisMixin that was missing before at metric_learn.rst. I don't know if there was some reason for this, but as BilinearMixin will be incorported in #329 and #333 is already merged adding pair_score and pair_distance, I think its important to have these classes in the docs. For instance, the user can notice from the docs that pair_distance is not implemented in BilinearMixin.

This PR is ready for a code review. Summary:

mvargas33 commented 3 years ago

Here is a quick preview of the Methods directive. Its a summary between the Attributes and the Method detailed descriptions, in every class.

mvargas33 commented 3 years ago

So I've removed the Method directive everywhere because the method summary can be generated automatically by sphinx autosummary, at a cost of having one warning per function in each class (lots of them).

I've hardly tried to keep the methods summary generation without warnings, but I failed to do so.

I think its ok for now to keep this kind of warnings, because hard-coding the Method directive is too much work, and less flexible while extending the code in the future.

mvargas33 commented 3 years ago

Closes #226

I've added some extra CSS to show deprecation warnings in red as requested in this other issue. The final result looks like this.

There is no extra work from now on, you can use .. deprecated:: directive directly and it will have the custom style.