improve coding style and documentation style in affine schemes

[bhutz]

There are number of coding style issues and documentation issues in the affine scheme functionality such as

• == False or == True

• references to self in docs

• no single line description

• formatting of errors

etc.

Component: algebraic geometry

Author: Rebecca Lauren Miller

Branch: 0ab95a6

Reviewer: Ben Hutz

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

[sagetrac-rlmiller]

Branch: u/rlmiller/ticket/19889

[sagetrac-rlmiller]

Author: Rebecca Miller

[sagetrac-rlmiller]

New commits:

95aeb38

19889 Improved documentation of affine schemes folder

[sagetrac-rlmiller]

Commit: 95aeb38

[sagetrac-git]

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

a65930e	Merge branch 'master' into ticket/19889
eda3eef	19889 Fixed the doc tests

[sagetrac-git]

Changed commit from 95aeb38 to eda3eef

[bhutz]

Reviewer: Ben Hutz

[bhutz]

comment:6

Most of these are minor. But there are couple places the code can be improved. All changes referenced by line number.

affine_space.py

• 44 - x
• 60 = n, R
• 223,224 - F
• 397+ -> polys should be v
• 419 - v
• 490 - R
• 525 - line too long
• 530 - line too long
• 646 - X
• 744 - self
• 747 - line too long
• 756 - self
• 814 - If sentence in new paragraph

affine_point.py

• 102 - empty line between
• 112 - ',space'
• 139 - line too long
• 146 - empty line between
• 177 - '=' (is this line even needed?)
• 241 - number field, number field order
• 293 - self
• 355-357 - can be OUTPUT: integer.
• 390 - line too long
• 405 - y*z (no space)
• 422 - ,space
• 428,429 - are these really needed

affine_morphism.py

• 174 - line too long
• 176 - line too long
• 192 - Better would be "if the two affine maps defined the same map), line too long
• 229 - same comment as 192
• 330 - line too long
• 343 - line too long
• 347 - line too long
• 358 - ,space
• 383 - line too long
• 439 - line too long
• 501 - line too long
• 583 - n
• 658 - n
• 662 - map's
• 663 - empty line between
• 667 - map's
• 703 - no blank line
• 709 - empty line between
• 743 - fix spaces around =
• 753 - ,space
• 755 - line too long
• 770 - space+space
• 780 - space+space
• 784 - this to do I think is done (Projective Point global_height remove special case for ZZ #15376), so you can probably remove the special case code.
• 813,814 - single `
• 824 - space-space
• 865 - empty line between
• 996 - line too long
• 1001 - map's
• 1027 - x
• 1066 - line too long, need single line description
• 1071 - OUTPUT: a digraph.

[jdemeyer]

comment:7

You shouldn't do this:

 r"""
-Set of homomorphisms between two affine schemes
+Set of homomorphisms between two affine schemes.

The title of a module should not end with a period.

[jdemeyer]

comment:8

Some changes go from good to bad I think. In these cases, I prefer the original code:

-from sage.arith.all import lcm, gcd
+from sage.arith.all                 import lcm, gcd

and

-            if not isinstance(polys, (list, tuple)):
+            if not isinstance(polys,(list, tuple)):

and

-        Construct a point.
+        Constructs a point.

[bhutz]

comment:9

I wasn't sure of the convention for the imports: have them all lined up or just single space after import?

Yes, those other changes should be un-done. I went through the files looking at what was potentially missed and didn't go through the specific changes yet.

[jdemeyer]

comment:10

The column style

from sage.arith.all                 import lcm, gcd

is annoying for several reasons:

1. it makes lines needlessly long
2. it breaks when you need from sage.some.module.with.a.really.very.long.name import foo.
3. it breaks easy search-and-replace: if I decide to change arith to arithmetic for example, I would have to fix the formatting

[bhutz]

comment:11

Those seem like good reasons for single space to me.

[sagetrac-git]

Changed commit from eda3eef to 1a86718

[sagetrac-git]

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

1a86718

19889 Fixing Style Errors

[bhutz]

comment:14

There is a minor merge conflict with the latest beta.

The docs do not build, so I wasn't able to look at them for formatting issues. However, I went through the changes are there are some specifics to fix:

undo

-            if not isinstance(polys, (list, tuple)):
+            if not isinstance(polys,(list, tuple)):

define - this occurs in two places

+        - Boolean - True if the two affine maps defined the same map.
+        - Boolean - True if the two affine maps defined the same map.

map's

+        - ``P`` -- a point in the map domain.

extra space

+            sage: A.<x,y> =  AffineSpace(FractionField(R), 2)

new paragraph for 2nd sentence

+        Returns the multiplier of the point ``P`` of period ``n`` by the map.
+        The map must be an endomorphism.

of the map instead of extension field

+            and codomain of extension field.

no period

+Scheme morphism for points on affine varieties.

undo

-        Returns the point `f^n(self)`
+        Returns the point `f^n(point)`

'with this point in' instead of 'self if'

+        - ``f`` -- a :class:`SchemeMorphism_polynomial` with ``self`` if ``f.domain()``.

2nd sentence new paragraph

+        Returns the orbit of the point by `f`. If `n` is an integer it returns
+        `[self,f(self), \ldots, f^{n}(self)]`.

missing space

+            sage: A.<x,y>=AffineSpace(QQ, 2)
+            sage: H = Hom(A, A)
+            sage: f = H([(x-2*y^2)/x,3*x*y])
+            sage: A(9, 3).orbit(f, 3)

another special case to remove + associated code

-            add heights to integer.pyx and remove special case
+            Add heights to integer.pyx and remove special case.

The 2nd sentence should be the 1st. The 1st sentence needs more info ('Since every...finite field, this pair always exists.')

+        Every point is preperiodic over a finite field.
+
+        This function returns the pair `[m, n]` where `m` is the
+        preperiod and `n` is the period of the point by ``f``.

[sagetrac-git]

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

d01f8c6

Merge branch 'master' into ticket/19889

[sagetrac-git]

Changed commit from 1a86718 to d01f8c6

[sagetrac-git]

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

730feee

19889 Fixed style of affine folder

[sagetrac-git]

Changed commit from d01f8c6 to 730feee

[bhutz]

comment:18

I'm still not getting the docs to build even after a make doc-clean. I get

[schemes  ] /home/ben/sage/sage-test/local/lib/python2.7/site-packages/sage/schemes/affine/affine_morphism.py:docstring of sage.schemes.affine.affine_morphism.SchemeMorphism_polynomial_affine_space.nth_iterate_map:1: WARNING: Inline literal start-string without end-string.
Error building the documentation.

It seems to be complaining about the

``n``th

making it

``n``-th

seems to fix the issue.

Another small change I missed last time.

• In point.py orbit. The parameter is N instead of n.

[sagetrac-git]

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

60eea2d

19889 Fixing Style of affine folder

[sagetrac-git]

Changed commit from 730feee to 60eea2d

[bhutz]

comment:20

I assume you meant to set this to needs review. The docs build now and I was able to look at them. It looks like some of your line wrapping for INPUT/OUTPUT is not formatting correctly. It is because of the number of spaces indenting the 2nd line. For example look at the input/output of dynatomic_polynomial in affine_morphism. The output formats correctly, but the input does not.

There are a number of such spacing issues in the files.

[sagetrac-git]

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

3e779d7	Merge branch 'master' into ticket/19889
0ab95a6	19889 Fxing style of affine folder

[sagetrac-git]

Changed commit from 60eea2d to 0ab95a6

[vbraun]

Changed branch from u/rlmiller/ticket/19889 to 0ab95a6

[jdemeyer]

Changed commit from 0ab95a6 to none

[jdemeyer]

comment:25

What do you prefer: "Rebecca Lauren Miller" or "Rebecca Miller"?

[sagetrac-rlmiller]

Changed author from Rebecca Miller to Rebecca Lauren Miller