#19891 closed enhancement (fixed)
improve coding style and documentation in projective schemes
Reported by:  bhutz  Owned by:  bhutz 

Priority:  trivial  Milestone:  sage7.1 
Component:  algebraic geometry  Keywords:  
Cc:  Merged in:  
Authors:  Ben Hutz  Reviewers:  Frédéric Chapoton, Rebecca Lauren Miller, Joseph Eisner 
Report Upstream:  N/A  Work issues:  
Branch:  64390f3 (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
There are number of coding style issues and documentation issues in the projective scheme functionality such as
 == False or == True
 references to self in docs
 no single line description
 formatting of errors
etc.
Change History (27)
comment:1 Changed 2 years ago by
 Branch set to u/bhutz/ticket/19891
comment:2 Changed 2 years ago by
 Commit set to 7aac718894d13b8f52373ec0f38d080dca3ed162
 Reviewers set to Frédéric Chapoton
comment:3 Changed 2 years ago by
 Commit changed from 7aac718894d13b8f52373ec0f38d080dca3ed162 to 24a7f72b936648d2fdc02941022bed78772d72d8
Branch pushed to git repo; I updated commit sha1. New commits:
24a7f72  19891: fix references and typos

comment:4 Changed 2 years ago by
 Status changed from new to needs_review
Thanks for taking a look at this, Frederic. Actually, I didn't put it as needsreview as I was going have my new students go over it this week to get used to the reviewing process since there are no significant/difficult changes here (and the references still weren't working correctly). Your review is much appreciated and any additional typos caught are important to fix. I've fixed the ones you found and I may as well set it to needsreview at this point.
That is good to know about the reference. I hadn't figured out yet why they were only linking for some of them, so you saved some time trying to determine what was happening.
comment:5 Changed 2 years ago by
 Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Rebecca Lauren Miller
 Status changed from needs_review to needs_work
Ticket 19891
no period for input or output consistency
file endPN_automorphism_group. html
 automorphism group QQ
Prime_lower_bound, formatting missing quotes automorism fixing pair
whole file
 some out put and input capitalized others not, some punch at end , others not
 capitalize output
periods after all hyperlinked ref?
file endPN_minimal_model.html
 func minimal model example spacing
 func affine minimal example spacing
file projective morphism.html
 func automosphism group need to link referencd [fmv] need link
 func dyatomic polyomial references [mopa] need link
? why trac ticket references?
 func pgl minimal references need link
 func minimal model references need link, is self necessary?
 input check, quotes or not seen both ways
 func multiplier spectra output qqbar fix( check all too!)
 func normalize coordinates note formating, and gcd? base_ring?
 func nth iterate, to do formatinf and random question in example
 fun periodic points output self?
 func primes of bad reduction groebner capitalize
 check all output nones
 func wronskian_ideal capitalize jacobian
 func all_rational preimages remove self
 function automrophism group link reference [fmv]
file projective_point. html
 func dehomogenize punctuation in description
 func nth iterate kwds: Boolean, capitalize
comment:6 Changed 2 years ago by
 Reviewers changed from Frédéric Chapoton, Rebecca Lauren Miller to Frédéric Chapoton, Rebecca Lauren Miller, Joseph Eisner
Verify you meant to remove the dehomogenize step:
@@ 372,31 +372,30 @@ class SchemeMorphism_polynomial_projective_space(SchemeMorphism_polynomial):
Should these two errors match?:
@@ 2271,13 +2284,13 @@ class SchemeMorphism_polynomial_projective_space(SchemeMorphism_polynomial):
Check line breaks:
@ 413,8 +415,10 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 850,9 +857,11 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
Grammar:
@@ 1501,8 +1514,10 @@ class SchemeMorphism_point_projective_field(SchemeMorphism_point_projective_ring
@@ 280,15 +280,16 @@ def automorphism_group_QQ_fixedpoints(rational_function, return_functions = Fals
Remove reference to self:
@@ 2659,16 +2680,18 @@ class SchemeMorphism_polynomial_projective_space(SchemeMorphism_polynomial):
@@ 3567,11 +3596,11 @@ class SchemeMorphism_polynomial_projective_space_field(SchemeMorphism_polynomial
@@ 973,7 +989,7 @@ class ProjectiveSpace_field(ProjectiveSpace_ring):
Make references hyperlinks:
@@ 4249,10 +4289,14 @@ class SchemeMorphism_polynomial_projective_space_finite_field(SchemeMorphism_pol
@@ 4291,14 +4335,15 @@ class SchemeMorphism_polynomial_projective_space_finite_field(SchemeMorphism_pol
@@ 2391,37 +2406,38 @@ class SchemeMorphism_polynomial_projective_space(SchemeMorphism_polynomial):
@@ 2360,8 +2374,9 @@ class SchemeMorphism_polynomial_projective_space(SchemeMorphism_polynomial):
Spacing Issues:
@@ 765,7 +764,7 @@ class SchemeMorphism_polynomial_projective_space(SchemeMorphism_polynomial):
@@ 803,16 +802,16 @@ class SchemeMorphism_polynomial_projective_space(SchemeMorphism_polynomial):
@@ 833,7 +832,7 @@ class SchemeMorphism_polynomial_projective_space(SchemeMorphism_polynomial):
@@ 4129,9 +4166,9 @@ class SchemeMorphism_polynomial_projective_space_finite_field(SchemeMorphism_pol
@@ 4159,7 +4199,7 @@ class SchemeMorphism_polynomial_projective_space_finite_field(SchemeMorphism_pol
@@ 45,9 +44,9 @@ def _fast_possible_periods(self,return_points=False):
@@ 102,59 +101,59 @@ def _fast_possible_periods(self,return_points=False):
@@ 182,58 +182,58 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 246,20 +246,20 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 293,58 +293,58 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 3010,12 +3039,14 @@ class SchemeMorphism_polynomial_projective_space(SchemeMorphism_polynomial):
@@ 565,53 +569,54 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 742,16 +749,16 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 759,8 +766,8 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 813,9 +820,9 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 882,20 +891,20 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 903,12 +912,12 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 1064,8 +1075,8 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 1075,8 +1086,8 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 1085,8 +1096,8 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 1176,14 +1187,14 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 1228,26 +1239,26 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 1262,30 +1273,31 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 1359,7 +1372,7 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 1368,10 +1381,10 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 1395,8 +1408,8 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 1406,7 +1419,7 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 1414,8 +1427,8 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
@@ 1424,24 +1437,24 @@ class SchemeMorphism_point_projective_ring(SchemeMorphism_point):
comment:7 Changed 2 years ago by
 Commit changed from 24a7f72b936648d2fdc02941022bed78772d72d8 to ab8ad1cbd1b0020d5908ef98db6acf033db11dd1
Branch pushed to git repo; I updated commit sha1. New commits:
ab8ad1c  19891: additional fixes from review.

comment:8 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:9 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:10 Changed 2 years ago by
 Status changed from positive_review to needs_work
Merge conflict, try with the next beta
comment:11 Changed 2 years ago by
 Commit changed from ab8ad1cbd1b0020d5908ef98db6acf033db11dd1 to 0fe9788511ecad1679f28a3d94032412e8811b24
Branch pushed to git repo; I updated commit sha1. New commits:
0fe9788  Merge branch 'master' into ticket/19891

comment:12 Changed 2 years ago by
that still wasn't the right beta, I have it now and did another merge. Conflict resolved; am now testing.
comment:13 Changed 2 years ago by
 Commit changed from 0fe9788511ecad1679f28a3d94032412e8811b24 to ae6b53dad8e6c164bd0c491abd8c58e60865edc8
Branch pushed to git repo; I updated commit sha1. New commits:
ae6b53d  Merge branch 'master' into ticket/19891

comment:15 Changed 2 years ago by
 Commit changed from ae6b53dad8e6c164bd0c491abd8c58e60865edc8 to d8752ed6a585e944085833f2ac826cea3e757d08
Branch pushed to git repo; I updated commit sha1. New commits:
d8752ed  19891: reduce spacing in imports

comment:16 Changed 2 years ago by
 Description modified (diff)
 Status changed from needs_review to positive_review
comment:17 Changed 2 years ago by
 Description modified (diff)
comment:18 Changed 2 years ago by
 Description modified (diff)
 Milestone changed from sage7.0 to sage7.1
Fixed description and updated milestone.
comment:19 Changed 2 years ago by
 Status changed from positive_review to needs_work
PDF docs don't build:
! Missing $ inserted. <inserted text> $ l.32465 \bibitem[Silverman_ADS]{Silverman_ADS} {\phantomsection\label{sage/sc...
You can't use underscores as bibitem keys
comment:20 Changed 2 years ago by
 Commit changed from d8752ed6a585e944085833f2ac826cea3e757d08 to bcf2bd1d01e250bbb5a06a2fabf3ed402d29b691
Branch pushed to git repo; I updated commit sha1. New commits:
bcf2bd1  19891: fix _ in references for pdf docs

comment:21 Changed 2 years ago by
 Status changed from needs_work to needs_review
oops. Sorry, I forgot about the no underscores and obviously didn't build the pdf version.
The pdf docs compile now.
comment:23 Changed 2 years ago by
 Branch changed from u/bhutz/ticket/19891 to u/jdemeyer/ticket/19891
comment:24 Changed 2 years ago by
 Commit changed from bcf2bd1d01e250bbb5a06a2fabf3ed402d29b691 to 64390f3ae9aea0bce82e9976b2508d12703c197b
 Status changed from positive_review to needs_review
Can you accept this small change to avoid a conflict with #20011?
comment:25 Changed 2 years ago by
 Status changed from needs_review to positive_review
yes, that is fine.
comment:26 Changed 2 years ago by
 Branch changed from u/jdemeyer/ticket/19891 to 64390f3ae9aea0bce82e9976b2508d12703c197b
 Resolution set to fixed
 Status changed from positive_review to closed
comment:27 Changed 2 years ago by
 Commit 64390f3ae9aea0bce82e9976b2508d12703c197b deleted
What do you prefer: "Rebecca Lauren Miller" or "Rebecca Miller"?
Please put this ticket in "needs_review". I have assumed that you forgot to do so. Here are some points that I have seen at first reading.
is wrong. The correct refererence syntax is
[Hutzgr]_
with an underscore at the end. This happen at many places, for example also forSee [FMV]
and[Silverman_ADS]
and[BruinMolnar] and [Molnar]
.could be
This looks strange:
Abelian variety
should keep its capital Ashould be
greens function
should beGreen's function
+ THis map must
should be+ This map must
typo
the subsgroup
at least twicetypo
postcritially
typo
the the LLL
duplicate reference
[Hutz_gr]
and[Hutzgr]
?typo
this projective spaces
New commits:
19891: fixes for endPN_minimal_model.py
19891: fixes for projective_homset.py
19891: fixes for endPN_automorphism_group.py
19891: fixes for projective_morphism.py
19891: fixes for projective_morphism_help.py and update references
19891 fixes for projective_point.py
19891: fixes for projective_rational_point.py
19891: fixes for projective_space.py
19891: doc formatting cleanup