Opened 21 months ago

Closed 19 months ago

Last modified 18 months ago

#19889 closed enhancement (fixed)

improve coding style and documentation style in affine schemes

Reported by: bhutz Owned by: rlmiller
Priority: trivial Milestone: sage-7.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 21 months ago by bhutz

  • 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 21 months ago by rlmiller

  • Owner changed from millerrl to rlmiller

comment:3 Changed 20 months ago by rlmiller

  • Branch set to u/rlmiller/ticket/19889

comment:4 Changed 20 months ago by rlmiller

  • Authors set to Rebecca Miller
  • Commit set to 95aeb38e5e3225b164d61f15ed70209634b5caf7
  • Status changed from new to needs_review

New commits:

95aeb3819889 Improved documentation of affine schemes folder

comment:5 Changed 20 months ago by git

  • Commit changed from 95aeb38e5e3225b164d61f15ed70209634b5caf7 to eda3eef58a5b921d0e5a775551c6f20c490430ce

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

a65930eMerge branch 'master' into ticket/19889
eda3eef19889 Fixed the doc tests

comment:6 Changed 20 months ago by bhutz

  • 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
  • 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 (#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.

comment:7 Changed 20 months ago by jdemeyer

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 20 months ago by jdemeyer

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 20 months ago by bhutz

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.

comment:10 Changed 20 months ago by jdemeyer

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

comment:11 Changed 20 months ago by bhutz

Those seem like good reasons for single space to me.

comment:12 Changed 20 months ago by git

  • Commit changed from eda3eef58a5b921d0e5a775551c6f20c490430ce to 1a86718a61fec39cb0328770318cf7d82f110e2e

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

1a8671819889 Fixing Style Errors

comment:13 Changed 20 months ago by rlmiller

  • Status changed from needs_work to needs_review

comment:14 Changed 20 months ago by bhutz

  • 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([(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``.

comment:15 Changed 20 months ago by git

  • Commit changed from 1a86718a61fec39cb0328770318cf7d82f110e2e to d01f8c634f0e6e2509f840c47fed01dad1f7fa9b

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

d01f8c6Merge branch 'master' into ticket/19889

comment:16 Changed 20 months ago by git

  • Commit changed from d01f8c634f0e6e2509f840c47fed01dad1f7fa9b to 730feeed69d5d837f6d6a41b9ce4d349f8e2cc05

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

730feee19889 Fixed style of affine folder

comment:17 Changed 20 months ago by rlmiller

  • Status changed from needs_work to needs_review

comment:18 Changed 20 months ago by bhutz

  • Milestone changed from sage-7.0 to sage-7.1
  • Status changed from needs_review to needs_work

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.

comment:19 Changed 20 months ago by git

  • Commit changed from 730feeed69d5d837f6d6a41b9ce4d349f8e2cc05 to 60eea2dcac360a0229fdcd6d513ce9c3989ec706

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

60eea2d19889 Fixing Style of affine folder

comment:20 Changed 20 months ago by bhutz

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 20 months ago by git

  • Commit changed from 60eea2dcac360a0229fdcd6d513ce9c3989ec706 to 0ab95a64c0338ef972174dfaee4cbc62c1d98f08

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

3e779d7Merge branch 'master' into ticket/19889
0ab95a619889 Fxing style of affine folder

comment:22 Changed 20 months ago by rlmiller

  • Status changed from needs_work to needs_review

comment:23 Changed 20 months ago by bhutz

  • Status changed from needs_review to positive_review

comment:24 Changed 19 months ago by vbraun

  • Branch changed from u/rlmiller/ticket/19889 to 0ab95a64c0338ef972174dfaee4cbc62c1d98f08
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:25 Changed 18 months ago by jdemeyer

  • Commit 0ab95a64c0338ef972174dfaee4cbc62c1d98f08 deleted

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

comment:26 Changed 18 months ago by rlmiller

  • Authors changed from Rebecca Miller to Rebecca Lauren Miller
Note: See TracTickets for help on using tickets.