Fix autofunction references for sphinx 3.1.1

[oscarbenjamin]

References to other Issues or PRs

This is a fix for #19551 on master but it should also be cherrypicked for 1.6.1

Brief description of what is fixed or changed

Other comments

Release Notes

NO ENTRY

[sympy-bot]

✅

Hi, I am the SymPy bot (v160). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

• No release notes entry will be added for this pull request.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->

This is a fix for #19551 on master but it should also be cherrypicked for 1.6.1

#### Brief description of what is fixed or changed


#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

[asmeurer]

Do we have to do this for every one? I thought it was just the ones that had conflicting names.

[oscarbenjamin]

Here I've just made changes to silence the warnings/errors from the build so I haven't changed all of them. Where I have changed them I've mostly just done it for the whole file. There are a lot of conflicting names in sympy:

from sympy.series.series import series

The ambiguity for series applies to everything in the series package. Same goes for matrices (because we have sympy.matrices and sympy.matrices.matrices) and solvers, integrals, logic, and some submodules like plotting.plot, printing.rcode etc.

There aren't many changes in simplify but I think that's because most references use:

.. currentmodule:: sympy.simplify.simplify

The other option is to make more use of currentmodule and related scoping directives but that won't help for cross-referencing.

[oscarbenjamin]

Some of these wouldn't need to be changed if we went for #19554

[gschintgen]

I only superficially watched this whole issue and my knowledge of .rst is rather limited. That's why I'm commenting here (but maybe I'm misunderstanding the issue): if there are two syntax choices and one of them is guaranteed to always work, then I'd definitely prefer to have the universal one applied on the whole codebase.
After all, how do I edit these files? I'll look at the syntax of the surrounding code and maybe some other files and re-use that same syntax.

[oscarbenjamin]

After all, how do I edit these files?

There is a style guide but it wasn't much help in this particular situation:
https://docs.sympy.org/latest/documentation-style-guide.html

I tend to agree that we should just make this change throughout the codebase. Here I've just gone for the minimal "make it work" fix so that it can be backported to 1.6.1 as well.

[oscarbenjamin]

Also using currentmodule works and is less verbose. That could be used instead of having sympy.solvers.diophantine.diophatine everywhere here:
https://raw.githubusercontent.com/sympy/sympy/master/doc/src/modules/solvers/diophantine.rst

[gschintgen]

There is a style guide but it wasn't much help in this particular situation:
https://docs.sympy.org/latest/documentation-style-guide.html

Yes, sorry if my comment was formulated a bit provocatively. I know and appreciate the style guide! But for small edits I often find myself lazily copying what I see. (I do check the actual html output though before pushing.)

[asmeurer]

I only superficially watched this whole issue and my knowledge of .rst is rather limited. That's why I'm commenting here (but maybe I'm misunderstanding the issue): if there are two syntax choices and one of them is guaranteed to always work, then I'd definitely prefer to have the universal one applied on the whole codebase.

I agree with this. However, I'm not yet convinced that this isn't just a Sphinx bug (sphinx-doc/sphinx#7841). To me, it should be sufficient to use the right "auto" directive (automodule, autofunction, autoclass, etc.). This also used to work before in an earlier version of Sphinx. So if Sphinx ends up changing it so that the old way continues to work, then we won't need the colons anymore. For now this should just be seen as a workaround.

If Sphinx doesn't change and the colons remain required, then I we should update the style guide to indicate that's what should be used.

[asmeurer]

The ambiguity for series applies to everything in the series package. Same goes for matrices (because we have sympy.matrices and sympy.matrices.matrices) and solvers, integrals, logic, and some submodules like plotting.plot, printing.rcode etc.

I see. I thought it was only functions and modules with the same name that caused the problem, which is why I was confused to see things like printing. But it also applies if a submodule has the same name as a parent module.

[oscarbenjamin]

To me, it should be sufficient to use the right "auto" directive (automodule, autofunction, autoclass, etc.).

The issue is not which directive you use. If you use

    .. autoclass:: sympy.series.order.Order

then sphinx understands that Order is a class. What sphinx doesn't know is that in the expression sympy.series.order.Order the series attribute of the sympy module is not the sympy.series package but is in fact the series function that is defined in the sympy.series.series module but has been imported through to the top level sympy/__init__.py overwriting the series package attribute that would be created by from .series import .... It seems that sphinx is making the assumption that imports can be replaced with attribute access but that is not true in general.

At the same time sympy's imports are a mess and it would be better if we didn't have so many conflicting names. Many of these conflicts are due to the deprecated imports which we could remove. For example the only reason that matrices is a problem is because sympy.matrices is the sympy.matrices.matrices submodule wrapped in a DeprecatedImport wrapper. The same is the cause of the problem for logic (sympy.core.logic), integrals and solvers.

We can just remove those (#19554). That wouldn't fix the whole problem though as you can see from the build failure there:
https://travis-ci.org/github/sympy/sympy/jobs/698197866#L4084

Most of the remaining problems after #19554 are to do with diophantine

>>> sympy.solvers.diophantine                                                                                                                             
<function sympy.solvers.diophantine.diophantine.diophantine(eq, param=t, syms=None, permute=False)>

and plotting.plot.plot(), assumptions.ask.ask() etc.

[oscarbenjamin]

But it also applies if a submodule has the same name as a parent module.

It's okay for a submodule to have the same name as a parent module provided we don't import that submodule under the same name into the grand-parent module. The same goes for having a function that has the same name as its module. The problem begins when the parent package's __init__.py does

from .series import series

Normally a relative import like this assigns the imported series module (".series") as an attribute of the current module but here it gets overwritten by the series function imported from that mdoule instead.

Before sympy 1.6 the main sympy/__init__.py used star imports for everything and so effectively overwrote any package whose name clashed with any submodule of any of the other packages. Hence sympy.core being sympy.core.core.

[asmeurer]

I guess there really is an ambiguity on Sphinx's side, since dots can also refer to attributes (e.g., sympy.series.order could be an object with an Order attribute). I guess the colons tell Sphinx that the first part should be treated like a dotted import and the rest as a name in that module.

But if after removing the deprecation stuff we don't need any colons, then I don't think we need to worry about it. Otherwise, I think we should note it in the style guide as something that is needed in some cases to remove ambiguity. We already note that in some instances you have to have a fully qualified name in a cross-reference (https://docs.sympy.org/latest/documentation-style-guide.html#cross-referencing). I see this as similar to that.

[codecov]

Codecov Report

Merging #19568 into master will increase coverage by 0.016%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##            master    #19568       +/-   ##
=============================================
+ Coverage   75.652%   75.668%   +0.016%     
=============================================
  Files          654       654               
  Lines       169941    169941               
  Branches     40065     40065               
=============================================
+ Hits        128564    128592       +28     
+ Misses       35764     35737       -27     
+ Partials      5613      5612        -1