Skip to content
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

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

Merged
merged 16 commits into from
Nov 17, 2021

Conversation

mvargas33
Copy link
Contributor

@mvargas33 mvargas33 commented Oct 14, 2021

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-gallery and 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 [DOC] Remove any flag #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:
  • By definition, while using the directive .. topic:: References: it means that something was referred previously. But in this case, this section is used as "find the original paper here" or "base code was taken from here" at the end of each algorithm description. None of the references is mention earlier in any text body, and because of that this kind of warning shows up: /metric-learn/doc/unsupervised.rst:39: WARNING: Footnote [1] is not referenced.
  • Also, in a single rst file the numeration of references must strictly increase, but in this case, each algorithm has references from 1-2 or 1-3, and because of that there are a lot of warnings of the form /metric-learn/doc/supervised.rst:378: WARNING: Duplicate explicit target name: "1". Which makes sense, because if I want to reference the target [1] in a text body, there are 6 or 7 references with that name.

Some possible solutions:

  • Don't use .. topic:: References: but just a list instead, this will delete all warnings.
  • Use the reference as they are intended to be used: use them in the text body of each algorithm, and use a unique incresing number id for each reference in the same file.

Besides this last problem, all warnings were fixed.

@bellet
Copy link
Member

bellet commented Oct 18, 2021

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
Copy link
Member

bellet commented Oct 18, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

mvargas33 commented Nov 2, 2021

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:

  • All warnings are removed from building the docs
  • Add Methods directive to all classes, wich is a summary of all methods from that class.
  • Changed CSS for Example Code and References
  • Update dependencies to build the docs (Actually they don't compile as they are before this PR)
  • Removed deprecated functions in conf.py (for js and css)
  • Removed any flag (Fixes [DOC] Remove any flag #305 )

@mvargas33 mvargas33 changed the title [DOC] Fixes most warnings in the docs [DOC] Fixes all warnings in the docs Nov 2, 2021
@mvargas33 mvargas33 changed the title [DOC] Fixes all warnings in the docs [MRG][DOC] Fixes all warnings in the docs Nov 2, 2021
doc/conf.py Outdated Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
@@ -285,6 +331,39 @@ class SDML_Supervised(_BaseSDML, TransformerMixin):
The linear transformation ``L`` deduced from the learned Mahalanobis
metric (See function `components_from_metric`.)

Methods
Copy link
Contributor

Choose a reason for hiding this comment

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

I may have missed it in your PR description, but why do we need a full method listing in each class docstring? I expect the doc building process to handle method listings automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expected that as well but it does not happen. So it all started because there is a lot of warnings like these:

/home/max/inria/metric-learn/metric_learn/sdml.py:docstring of metric_learn.sdml.SDML_Supervised:82: WARNING: autosummary: stub file not found 'metric_learn.SDML_Supervised.get_params'. Check your autosummary_generate setting.

See these image as reference.

And I've found the solution in stackoverflow from a person that was also extending from the Estimator class and had exactly the same errors. So I followed his advice and added the Method directive in every class, and that removed all these bunch of warnings.

Without explicitly adding this directive, no method summary is shown in the docs, just the detailed method list.

I also thought that the Method directive could be incremental, so for each class I only had to add the methods description of that class, and not the method description of his parent, and grandparent class as well. But sadly it doesn't work that way either.

It may look like an overkill right now, but thinking about it, when someone has to add another algorithm in the future, you already have the docstrings from other algorithms as reference, so it won't be overwhelming.

Also, if someone wants to extend the docs, it's better for them to see the warnings/errors that only they are producing, because right now its really really hard to debug the docs with all these warnings. @wdevazelhes says it at #305 as well (in this case for the 'any' flag, the second bunch of warnings).

Copy link
Member

@bellet bellet Nov 4, 2021

Choose a reason for hiding this comment

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

Regarding this: how about the second answer in the stackoverflow post you linked: https://stackoverflow.com/questions/65198998/sphinx-warning-autosummary-stub-file-not-found-for-the-methods-of-the-class-c
This could provide a way to remove the warnings. We would not have any method summary, but we have the list afterwards, so it is not a big deal considering the effort required to have the method summary. Of course the best solution would be that the summary is constructed automatically, but you seem to say that it is not possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been hours looking to build the methods summary automatically without any warning, without success.

We either use numpydoc_show_class_members = False removing the warnings but loosing the automatic method summary, or

We keep the implicit numpydoc_show_class_members = True generating the automatic method summary but with a bunch of warnings.

Yes, I realized that the method summary its actually being generated with the docs as they are. So I will remove the hard coded method summary that I've made, as it's a lot of work and can be done automatically, despite the warnings.

@mvargas33
Copy link
Contributor Author

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

Copy link
Member

@bellet bellet left a comment

Choose a reason for hiding this comment

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

Here are some additional comments

doc/_static/css/styles.css Outdated Show resolved Hide resolved
doc/supervised.rst Outdated Show resolved Hide resolved
Comment on lines +238 to +241
.. rubric:: References


.. container:: hatnote hatnote-gray
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice it there was a way to define a custom rubric or something so we only need to write something like
.. rubric:: References
and the appropriate CSS style would be automatically applied

Copy link
Member

Choose a reason for hiding this comment

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

(no big deal if there is no easy way to do this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After researching for while, there is not a clear path to this at all. Also, I think that even if its possible all .. rubric:: stuff will have the same CSS, including the Examples, so I guess its not a good idea to have this CSS globally, because we don't want grey boxes everywhere, just in the references.

@bellet bellet linked an issue Nov 4, 2021 that may be closed by this pull request
@mvargas33
Copy link
Contributor Author

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 mvargas33 changed the title [MRG][DOC] Fixes all warnings in the docs [MRG][DOC] Fixes almost all warnings in the docs Nov 10, 2021
@mvargas33
Copy link
Contributor Author

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.

Copy link
Member

@bellet bellet 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 a lot!

@bellet bellet merged commit 4e0c444 into scikit-learn-contrib:master Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC] Remove any flag deprecations should appear in red
3 participants