Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#19891 closed enhancement (fixed)

improve coding style and documentation in projective schemes

Reported by: bhutz Owned by: bhutz
Priority: trivial Milestone: sage-7.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 bhutz)

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 3 years ago by bhutz

  • Branch set to u/bhutz/ticket/19891

comment:2 Changed 3 years ago by chapoton

  • Commit set to 7aac718894d13b8f52373ec0f38d080dca3ed162
  • Reviewers set to Frédéric Chapoton

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.

+    See [Hutz-gr]

is wrong. The correct refererence syntax is [Hutz-gr]_ with an underscore at the end. This happen at many places, for example also for See [FMV] and [Silverman_ADS] and [Bruin-Molnar] and [Molnar].

`PGL(2,QQ)`

could be

`PGL(2,\QQ)`

This looks strange:

-        lift,lifted = blift(LG,Li,p,S=S)
+        lift,lifted = blift(LG,Li,p,S = S)

Abelian variety should keep its capital A

+        returns ``True`` if this map is a morphism.

should be

+        Return ``True`` if this map is a morphism.

greens function should be Green's function

+ THis map must should be + This map must

typo the subsgroup at least twice

typo post-critially

typo the the LLL

duplicate reference [Hutz_gr] and [Hutz-gr] ?

typo this projective spaces


New commits:

0db500b19891: fixes for endPN_minimal_model.py
9fcac6719891: fixes for projective_homset.py
293dd6c19891: fixes for endPN_automorphism_group.py
069751e19891: fixes for projective_morphism.py
8c073bc19891: fixes for projective_morphism_help.py and update references
d90476d19891 fixes for projective_point.py
935c0ec19891: fixes for projective_rational_point.py
ce4dac119891: fixes for projective_space.py
7aac71819891: doc formatting clean-up

comment:3 Changed 3 years ago by git

  • Commit changed from 7aac718894d13b8f52373ec0f38d080dca3ed162 to 24a7f72b936648d2fdc02941022bed78772d72d8

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

24a7f7219891: fix references and typos

comment:4 Changed 3 years ago by bhutz

  • Authors set to Ben Hutz
  • Status changed from new to needs_review

Thanks for taking a look at this, Frederic. Actually, I didn't put it as needs-review 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 needs-review 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 3 years ago by rlmiller

  • 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 3 years ago by eisnerj

  • 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 3 years ago by git

  • Commit changed from 24a7f72b936648d2fdc02941022bed78772d72d8 to ab8ad1cbd1b0020d5908ef98db6acf033db11dd1

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

ab8ad1c19891: additional fixes from review.

comment:8 Changed 3 years ago by bhutz

  • Status changed from needs_work to needs_review

comment:9 Changed 3 years ago by rlmiller

  • Status changed from needs_review to positive_review

comment:10 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict, try with the next beta

comment:11 Changed 3 years ago by git

  • Commit changed from ab8ad1cbd1b0020d5908ef98db6acf033db11dd1 to 0fe9788511ecad1679f28a3d94032412e8811b24

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

0fe9788Merge branch 'master' into ticket/19891

comment:12 Changed 3 years ago by bhutz

that still wasn't the right beta, I have it now and did another merge. Conflict resolved; am now testing.

comment:13 Changed 3 years ago by git

  • Commit changed from 0fe9788511ecad1679f28a3d94032412e8811b24 to ae6b53dad8e6c164bd0c491abd8c58e60865edc8

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

ae6b53dMerge branch 'master' into ticket/19891

comment:14 Changed 3 years ago by bhutz

  • Status changed from needs_work to needs_review

conflict resolved

comment:15 Changed 3 years ago by git

  • Commit changed from ae6b53dad8e6c164bd0c491abd8c58e60865edc8 to d8752ed6a585e944085833f2ac826cea3e757d08

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

d8752ed19891: reduce spacing in imports

comment:16 Changed 3 years ago by rlmiller

  • Description modified (diff)
  • Status changed from needs_review to positive_review

comment:17 Changed 3 years ago by rlmiller

  • Description modified (diff)

comment:18 Changed 3 years ago by bhutz

  • Description modified (diff)
  • Milestone changed from sage-7.0 to sage-7.1

Fixed description and updated milestone.

comment:19 Changed 3 years ago by vbraun

  • 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 3 years ago by git

  • Commit changed from d8752ed6a585e944085833f2ac826cea3e757d08 to bcf2bd1d01e250bbb5a06a2fabf3ed402d29b691

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

bcf2bd119891: fix _ in references for pdf docs

comment:21 Changed 3 years ago by bhutz

  • 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:22 Changed 3 years ago by rlmiller

  • Status changed from needs_review to positive_review

PDF's build.

comment:23 Changed 3 years ago by jdemeyer

  • Branch changed from u/bhutz/ticket/19891 to u/jdemeyer/ticket/19891

comment:24 Changed 3 years ago by jdemeyer

  • 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 3 years ago by bhutz

  • Status changed from needs_review to positive_review

yes, that is fine.

comment:26 Changed 3 years ago by vbraun

  • 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 jdemeyer

  • Commit 64390f3ae9aea0bce82e9976b2508d12703c197b deleted

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

Note: See TracTickets for help on using tickets.