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

Silence some docbuilding warnings #33735

Closed
jhpalmieri opened this issue Apr 20, 2022 · 43 comments
Closed

Silence some docbuilding warnings #33735

jhpalmieri opened this issue Apr 20, 2022 · 43 comments

Comments

@jhpalmieri
Copy link
Member

We remove some documents from the reference manual. The corresponding Python files were deprecated in previous tickets, and as part of the deprecation, most of the content and all of the documentation was removed. See sage.symbolic.getitem for a typical example. These pages now result in a useless page in the reference manual and a message Warning: Missing title for sage.symbolic.getitem when creating the documentation. This happens for the pages

For a future ticket: we need to remember to remove pages from the reference manual when currently deprecated modules/packages are actually removed, in particular sage.media, sage.finance, and sage.structure.graphics_file. These currently produce warning messages during docbuilding, but there is actual content in the documentation, so we keep them in place.

Other warnings which could be investigated:

DeprecationWarning: Passing a schema to Validator.iter_errors is deprecated and will be removed in a future release. Call validator.evolve(schema=new_schema).iter_errors(...) instead.

and

[IPKernelApp] WARNING | debugpy_stream undefined, debugging will not be enabled

and

FutureWarning: EllipticCurveHom_composite is experimental code.
See https://trac.sagemath.org/32744 for details.

and

...../sage/plot/contour_plot.py:206: 
UserWarning: No contour levels were found within the data range.
[plotting ]   CS = subplot.contour(self.xy_data_array, contours, cmap=cmap,

This was introduced intentionally in #21042, I think, but should we silence the warning?

Depends on #33738

Component: documentation

Author: John Palmieri, Matthias Koeppe

Branch/Commit: bcad832

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/33735

@jhpalmieri

This comment has been minimized.

@jhpalmieri

This comment has been minimized.

@jhpalmieri

This comment has been minimized.

@jhpalmieri

This comment has been minimized.

@jhpalmieri
Copy link
Member Author

@jhpalmieri
Copy link
Member Author

Commit: f748915

@jhpalmieri
Copy link
Member Author

comment:6

This branch removes many of these warnings. For reasons I don't understand, there is still one instance of UserWarning: The soupsieve package is not installed... and one of warnings.warn( remaining.


New commits:

bb3d77btrac 33735: remove some deprecated (and now undocumented) modules from the ref manual
f748915trac 33735: remove soupsieve warning messages

@jhpalmieri

This comment has been minimized.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 23, 2022

comment:8

The soupsieve warning will go away with #33738

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 23, 2022

Dependencies: #33738

@tscrim
Copy link
Collaborator

tscrim commented Apr 25, 2022

comment:10

I am not sure I support removing deprecated modules from the doc. I think they should be removed at the same time (with the doc saying they are deprecated).

@jhpalmieri
Copy link
Member Author

comment:11

Replying to @tscrim:

I am not sure I support removing deprecated modules from the doc. I think they should be removed at the same time (with the doc saying they are deprecated).

I haven't made any changes to the items in the third bullet point. I removed the items in the second, but they all look like this: sage.symbolic.getitem page.

@jhpalmieri
Copy link
Member Author

comment:12

Replying to @mkoeppe:

The soupsieve warning will go away with #33738

Should I remove that part of this ticket?

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 25, 2022

comment:13

Yes, I think so

@jhpalmieri

This comment has been minimized.

@jhpalmieri

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 25, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8cc1610trac 33735: remove some deprecated (and now undocumented) modules from the ref manual

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 25, 2022

Changed commit from f748915 to 8cc1610

@jhpalmieri
Copy link
Member Author

comment:17

Here is a new trimmed down version.

@tscrim
Copy link
Collaborator

tscrim commented Apr 26, 2022

comment:18

Replying to @jhpalmieri:

Replying to @tscrim:

I am not sure I support removing deprecated modules from the doc. I think they should be removed at the same time (with the doc saying they are deprecated).

I haven't made any changes to the items in the third bullet point. I removed the items in the second, but they all look like this: sage.symbolic.getitem page.

Indeed, I agree it should be fixed. However, I propose doing it by adding a proper docstring with a title that says it is deprecated.

@jhpalmieri
Copy link
Member Author

comment:19

Replying to @tscrim:

Replying to @jhpalmieri:

Replying to @tscrim:

I am not sure I support removing deprecated modules from the doc. I think they should be removed at the same time (with the doc saying they are deprecated).

I haven't made any changes to the items in the third bullet point. I removed the items in the second, but they all look like this: sage.symbolic.getitem page.

Indeed, I agree it should be fixed. However, I propose doing it by adding a proper docstring with a title that says it is deprecated.

Are you talking about deprecated modules like sage.media which still have proper docstrings, but which might say that they are deprecated, or are you talking about the ones with no current content? I hope that in the second case, the people who did the deprecation paid attention to what needed to be documented at the time and just forgot to clean up the reference manual. Further, the relevant tickets were closed 7 months ago and were in Sage 9.5, released at the end of January. So it will likely be 3 months with the documentation in its current stage, and I just don't believe that it's worth restoring them. Have there been any complaints about these pages?

That is, I prefer my approach. If you want to create docstrings for those files, I think it's a waste of time, but don't let that stop you.

@tscrim
Copy link
Collaborator

tscrim commented Apr 26, 2022

comment:20

Replying to @jhpalmieri:

Replying to @tscrim:

Replying to @jhpalmieri:

Replying to @tscrim:

I am not sure I support removing deprecated modules from the doc. I think they should be removed at the same time (with the doc saying they are deprecated).

I haven't made any changes to the items in the third bullet point. I removed the items in the second, but they all look like this: sage.symbolic.getitem page.

Indeed, I agree it should be fixed. However, I propose doing it by adding a proper docstring with a title that says it is deprecated.

Are you talking about deprecated modules like sage.media which still have proper docstrings, but which might say that they are deprecated, or are you talking about the ones with no current content? I hope that in the second case, the people who did the deprecation paid attention to what needed to be documented at the time and just forgot to clean up the reference manual. Further, the relevant tickets were closed 7 months ago and were in Sage 9.5, released at the end of January. So it will likely be 3 months with the documentation in its current stage, and I just don't believe that it's worth restoring them. Have there been any complaints about these pages?

I was thinking of the former case. Files with truely no content should be deleted altogether, but I am guessing you mean no docstring content. All I am thinking is having a title of something like:

"""
Media (deprecated)
"""

Nothing else is needed beyond this (other than possibly adding a ref to the deprecating ticket).

That is, I prefer my approach. If you want to create docstrings for those files, I think it's a waste of time, but don't let that stop you.

These docstrings last a bit longer on the the Sage doc since they only get updated between major versions. Thus, it is possible it lasts longer than the files within beta versions of Sage.

Unfortunately I don't have my computer with me for the next few days. I won't be able to push something until Sunday.

@jhpalmieri
Copy link
Member Author

comment:21

Replying to @tscrim:

Replying to @jhpalmieri:

Are you talking about deprecated modules like sage.media which still have proper docstrings, but which might say that they are deprecated, or are you talking about the ones with no current content? I hope that in the second case, the people who did the deprecation paid attention to what needed to be documented at the time and just forgot to clean up the reference manual. Further, the relevant tickets were closed 7 months ago and were in Sage 9.5, released at the end of January. So it will likely be 3 months with the documentation in its current stage, and I just don't believe that it's worth restoring them. Have there been any complaints about these pages?

I was thinking of the former case.

Good, that makes sense.

Files with truely no content should be deleted altogether, but I am guessing you mean no docstring content.

The pages I'm currently removing from the reference manual come from Python files that look like, for example (symbolic/getitem.py):

from sage.misc.lazy_import import lazy_import
lazy_import('sage.symbolic.expression',
            ['normalize_index_for_doctests', 'OperandsWrapper', 'restore_op_wrapper'],
            deprecation=32386)

(That's the entire file.)

All I am thinking is having a title of something like:

"""
Media (deprecated)
"""

Nothing else is needed beyond this (other than possibly adding a ref to the deprecating ticket).

That could be done on this ticket or on a future ticket. Could the deprecation function somehow take care of this automatically, when you're deprecating a module or a package?

Anyway, as of now:

  • sage.media.wav already has a line in its top-level docstring saying "This module (and all of sage.media) is deprecated."
  • sage.structure.graphics_file has no such message.
  • sage.finance doesn't have any such messages, either.

We could change the reference manual .rst files instead:

diff --git a/src/doc/en/reference/finance/index.rst b/src/doc/en/reference/finance/index.rst
index 28bddb84cd..d47df818e2 100644
--- a/src/doc/en/reference/finance/index.rst
+++ b/src/doc/en/reference/finance/index.rst
@@ -1,6 +1,8 @@
 Quantitative Finance
 ======================
 
+This module is deprecated.
+
 .. toctree::
    :maxdepth: 2
 
diff --git a/src/doc/en/reference/misc/index.rst b/src/doc/en/reference/misc/index.rst
index 801af043f4..e1ac8e97e6 100644
--- a/src/doc/en/reference/misc/index.rst
+++ b/src/doc/en/reference/misc/index.rst
@@ -71,6 +71,8 @@ Database Access
 Media
 ~~~~~
 
+These modules are deprecated.
+
 .. toctree::
    :maxdepth: 1
 

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 26, 2022

comment:22

Replying to @jhpalmieri:

The pages I'm currently removing from the reference manual come from Python files that look like, for example (symbolic/getitem.py):

from sage.misc.lazy_import import lazy_import
lazy_import('sage.symbolic.expression',
            ['normalize_index_for_doctests', 'OperandsWrapper', 'restore_op_wrapper'],
            deprecation=32386)

(That's the entire file.)

My mistake when working on the pynac integration ticket. I'll add the missing titles.

@jhpalmieri
Copy link
Member Author

comment:23

Do we need the missing titles restored, or is it better to just remove the pages from the reference manual? There is no actual content in the file from the point of view of documentation.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 26, 2022

comment:24

I don't really have an opinion on this, just currently these files violate our style guide

@tscrim
Copy link
Collaborator

tscrim commented Apr 26, 2022

comment:25

Replying to @mkoeppe:

I don't really have an opinion on this, just currently these files violate our style guide

IMO, we want the files there with the title in the documentation (and a deprecation message) because we should be documenting all of our files and changes. Even though it is deprecated, I don’t think it is good for a project to have “hidden” files.

This should be done on this ticket IMO as adding it back on.a future ticket would effectively be reverting this one.

@jhpalmieri
Copy link
Member Author

comment:26

We already omit some Python files from the reference manual. For example we routinely omit files like BLAH/all.py, and there are other examples. Anyone who adds a major piece to Sage can decide to include, or not, the various component pieces; for example, several files in modules/fp_graded/steenrod are not in the reference manual, and I think it's not fair to call those "hidden"; rather, the authors used editorial discretion to decide whether something warranted inclusion. The recently deprecated files under discussion now just contain a deprecation warning and some import statements, and so they are an awful lot like all.py files.

I don't object to the files matching our style guidelines and having a title, but we don't need to keep them in the reference manual, and their presence there serves no purpose that I can see. Anyone who is interested enough to know what's going on with them will be able to understand the deprecation message. That is, they are documented, just not in the reference manual.

@tscrim
Copy link
Collaborator

tscrim commented Apr 26, 2022

comment:27

I would argue that those files (other than all.py, which are special and we have some general guidelines, or at least practices/conventions, about those) should be included in the reference manual. They are describing Sage’s functionality in a public-facing way. In this case, these files were previously there, but they would be apparently disappearing without a deprecation from that perspective.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 27, 2022

@jhpalmieri
Copy link
Member Author

New commits:

9132f37src/sage/libs/pynac, src/sage/symbolic: Add titles to deprecated modules

@jhpalmieri

This comment has been minimized.

@jhpalmieri
Copy link
Member Author

Changed commit from 8cc1610 to 9132f37

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 28, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

bcad832src/sage/finance/time_series.py: Add title to deprecated module

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 28, 2022

Changed commit from 9132f37 to bcad832

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 28, 2022

Changed author from John Palmieri to John Palmieri, Matthias Koeppe

@tscrim
Copy link
Collaborator

tscrim commented Apr 29, 2022

comment:33

Thank you Matthias.

Are there any other issues to address? I am ready to set a positive review.

@tscrim
Copy link
Collaborator

tscrim commented Apr 29, 2022

Reviewer: Travis Scrimshaw

@jhpalmieri
Copy link
Member Author

comment:34

I am happy to move ahead and deal with any other issues, as they arise, on future tickets.

@tscrim
Copy link
Collaborator

tscrim commented Apr 29, 2022

comment:35

Then positive review.

@jhpalmieri
Copy link
Member Author

comment:36

Thanks!

@vbraun
Copy link
Member

vbraun commented May 12, 2022

Changed branch from u/mkoeppe/silence-docbuild-warnings to bcad832

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants