#19889 closed enhancement (fixed)
improve coding style and documentation style in affine schemes
Reported by:  bhutz  Owned by:  rlmiller 

Priority:  trivial  Milestone:  sage7.1 
Component:  algebraic geometry  Keywords:  
Cc:  Merged in:  
Authors:  Rebecca Lauren Miller  Reviewers:  Ben Hutz 
Report Upstream:  N/A  Work issues:  
Branch:  0ab95a6 (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description
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.
Change History (26)
comment:1 Changed 3 years ago by
 Summary changed from Fix coding style and documentation style in affine schemes to improve coding style and documentation style in affine schemes
 Type changed from task to enhancement
comment:2 Changed 3 years ago by
 Owner changed from millerrl to rlmiller
comment:3 Changed 3 years ago by
 Branch set to u/rlmiller/ticket/19889
comment:4 Changed 3 years ago by
 Commit set to 95aeb38e5e3225b164d61f15ed70209634b5caf7
 Status changed from new to needs_review
comment:5 Changed 3 years ago by
 Commit changed from 95aeb38e5e3225b164d61f15ed70209634b5caf7 to eda3eef58a5b921d0e5a775551c6f20c490430ce
comment:6 Changed 3 years ago by
 Reviewers set to Ben Hutz
 Status changed from needs_review to needs_work
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
 355357  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 (#15376), so you can probably remove the special case code.
 813,814  single `
 824  spacespace
 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.
comment:7 Changed 3 years ago by
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.
comment:8 Changed 3 years ago by
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.
comment:9 Changed 3 years ago by
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 undone. I went through the files looking at what was potentially missed and didn't go through the specific changes yet.
comment:10 Changed 3 years ago by
The column style
from sage.arith.all import lcm, gcd
is annoying for several reasons:
 it makes lines needlessly long
 it breaks when you need
from sage.some.module.with.a.really.very.long.name import foo
.  it breaks easy searchandreplace: if I decide to change
arith
toarithmetic
for example, I would have to fix the formatting
comment:11 Changed 3 years ago by
Those seem like good reasons for single space to me.
comment:12 Changed 3 years ago by
 Commit changed from eda3eef58a5b921d0e5a775551c6f20c490430ce to 1a86718a61fec39cb0328770318cf7d82f110e2e
Branch pushed to git repo; I updated commit sha1. New commits:
1a86718  19889 Fixing Style Errors

comment:13 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:14 Changed 3 years ago by
 Status changed from needs_review to needs_work
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([(x2*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``.
comment:15 Changed 3 years ago by
 Commit changed from 1a86718a61fec39cb0328770318cf7d82f110e2e to d01f8c634f0e6e2509f840c47fed01dad1f7fa9b
Branch pushed to git repo; I updated commit sha1. New commits:
d01f8c6  Merge branch 'master' into ticket/19889

comment:16 Changed 3 years ago by
 Commit changed from d01f8c634f0e6e2509f840c47fed01dad1f7fa9b to 730feeed69d5d837f6d6a41b9ce4d349f8e2cc05
Branch pushed to git repo; I updated commit sha1. New commits:
730feee  19889 Fixed style of affine folder

comment:17 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:18 Changed 3 years ago by
 Milestone changed from sage7.0 to sage7.1
 Status changed from needs_review to needs_work
I'm still not getting the docs to build even after a make docclean. I get
[schemes ] /home/ben/sage/sagetest/local/lib/python2.7/sitepackages/sage/schemes/affine/affine_morphism.py:docstring of sage.schemes.affine.affine_morphism.SchemeMorphism_polynomial_affine_space.nth_iterate_map:1: WARNING: Inline literal startstring without endstring. 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.
comment:19 Changed 3 years ago by
 Commit changed from 730feeed69d5d837f6d6a41b9ce4d349f8e2cc05 to 60eea2dcac360a0229fdcd6d513ce9c3989ec706
Branch pushed to git repo; I updated commit sha1. New commits:
60eea2d  19889 Fixing Style of affine folder

comment:20 Changed 3 years ago by
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.
comment:21 Changed 3 years ago by
 Commit changed from 60eea2dcac360a0229fdcd6d513ce9c3989ec706 to 0ab95a64c0338ef972174dfaee4cbc62c1d98f08
comment:22 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:23 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:24 Changed 3 years ago by
 Branch changed from u/rlmiller/ticket/19889 to 0ab95a64c0338ef972174dfaee4cbc62c1d98f08
 Resolution set to fixed
 Status changed from positive_review to closed
comment:25 Changed 3 years ago by
 Commit 0ab95a64c0338ef972174dfaee4cbc62c1d98f08 deleted
What do you prefer: "Rebecca Lauren Miller" or "Rebecca Miller"?
New commits:
19889 Improved documentation of affine schemes folder