Opened 3 years ago
Closed 2 years ago
#17282 closed enhancement (fixed)
Implementing Wehler K3 Surfaces
Reported by:  jdefaria  Owned by:  

Priority:  major  Milestone:  sage6.8 
Component:  algebraic geometry  Keywords:  
Cc:  Merged in:  
Authors:  Joao Alberto de Faria  Reviewers:  Ben Hutz, Grayson Jorgenson 
Report Upstream:  N/A  Work issues:  
Branch:  08877a4 (Commits)  Commit:  08877a497294020d4b090daba982560fe2b5b4be 
Dependencies:  Stopgaps: 
Description
Adding code for Wehler K3 surfaces, along with some useful tests regarding their properties
Change History (35)
comment:1 Changed 3 years ago by
 Branch set to u/jdefaria/ticket/17282
 Created changed from 11/03/14 15:42:40 to 11/03/14 15:42:40
 Modified changed from 11/03/14 15:42:40 to 11/03/14 15:42:40
comment:2 Changed 3 years ago by
 Commit set to 3b7005d3330aed23b09eed04ff82037d3b7caf7b
comment:3 Changed 3 years ago by
 Commit changed from 3b7005d3330aed23b09eed04ff82037d3b7caf7b to c263b79aae1c28982248f7548ba65fe20b5fae1e
Branch pushed to git repo; I updated commit sha1. New commits:
c263b79  Finished fixing documenation

comment:4 Changed 3 years ago by
 Milestone changed from sage6.4 to sage6.6
 Status changed from new to needs_review
comment:5 Changed 3 years ago by
 Commit changed from c263b79aae1c28982248f7548ba65fe20b5fae1e to 5e79368ba628a26e2ef0ca0ac62f48edfefcfe37
Branch pushed to git repo; I updated commit sha1. New commits:
5e79368  Fixed formating in documentation

comment:6 Changed 3 years ago by
 Commit changed from 5e79368ba628a26e2ef0ca0ac62f48edfefcfe37 to 9b268f823c0e0ff35f0b881dadaa50ca72bfe875
Branch pushed to git repo; I updated commit sha1. New commits:
9b268f8  Added references and fixed long lines

comment:7 Changed 3 years ago by
 Reviewers set to Ben Hutz, Grayson Jorgenson
Started reviewing. Done through degenerate_fibers(). Will continue later.
From docbuild:
 top of file todo block not formatted correctly
 degenerate_fibers() 
instead of
 is_degenerate()  # lines
 is_isomorphic()  other > right
 lambda_plus()  examples not formatted correctly
 sigmax()  P2 x 'P2 should not have '
 orbit_phi(), orbit_psi()  input not formatted correctly
From code:
 WehlerK3Surface: check that
polys
is a list or tuple
 randomWehlerK3:
 replace
getmon
withxmrange([3,3])
 Why can we choose L to be the diagonal bilinear polynomial
 replace
WehlerK3Surface_ring:
 init():
 get rid of ZZ(2),ZZ(2)  need to change ProductProjectiveSpace_init
 assert all(x.parent() is ZZ for x in N) to N=[Integer(x) for x in N]
 move TypeError? check line 142 to line 135
 _morphism(): delete since it just inherits from parent
 change_ring() check not needed
 Sxa: condense the 4 lines
AS = self.ambient_space() ASC = AS.coordinate_ring() PSY = AS[1]; PSYC = PSY.coordinate_ring()
to
PSY = self.ambient_space()[1]
 Lx*, Qx*, Sx*  need to check that a is a point in P^{2 }
 Ramification_poly():
 need to wrap return
 need to put in definition in documentation
 is_degenerate(): delete comments line 675677
 degenerate_fibers():
 line 772794: should use
rational_points
on subschemes from ticket 18008  line 803825: should use
rational_points
on subschemes from ticket 18008
 line 772794: should use
 degenerate_primes(): document the algorithm
 rename:
badPrimes
 todo  over number fields
 rename:
comment:8 Changed 2 years ago by
Review continues. All that is left after this block is the review the sigmaX and sigmaY for degenerate fibers
is_smooth():
 Algorithm statement: is not a sentence
 remove extra spaces (line 940,941)
sigmaX:
 put doc string on new line (line 964). Better description in docstring
 [CaSi?] does not enmpty lines around it
 Input: Point on surface in P^{2 \times P}2
 make check like call in projective morphism
 1024: can just be else instead of elif
 remove extra spaces 987988
 remove s= from degenerate examples
sigmaY:
 Better description in docstring
 [CaSi?] does not enmpty lines around it
 Input: Point on surface in P^{2 \times P}2
 make check like call in projective morphism
 remove extra spaces 11701171
 remove s= from degenerate examples
 1205: else instead of elif
phi:
 [CaSi?] does not enmpty lines around it
 Input: Point on surface in P^{2 \times P}2
 1362: the point coordatines should be a list.
 remove extra spaces 13581359
 check can be false on the 2nd call
psi:
 white space around check = True
 [CaSi?] does not enmpty lines around it
 Input: Point on surface in P^{2 \times P}2
 1362: the point coordatines should be a list.
 remove extra spaces 13941395
 check can be false on the 2nd call
lamba_plus:
 [CaSi?] citation in description
 give a description of the algorithm
P
 a surface point
 remove extra spaces 14341435
 14521453, 14551456  less indendation
 rename localHeight  no CamelCase
 14721477 use GPoly(1,l) do not need the ifs. Same for all of the 'if j ==' (1479  1496 > one line)
 should the *Q[0][i]/Q[0][k] be PK not Q (as in 1508)
 reduce 15001524 to one line
 add some comments
 is 1529 using newQ so that the coordinates don't blow up??
lamba_minus:
 all the same comments
canonical_height_plus:
 [CaSi?] citation in description
 give a description of the algorithm
P
 a surface point
 1699: missing [] around coordinates
 1708: one less indent
canonical_height_minus:
 all the same comments
canonical_height:
 better description
 give a description of the algorithm
P
 a surface point
 remove some extra spaces in examples
 1817  need [] around coordinate points
Fibers:
 The name of the function Fibers shouldn't be plural
 Fix the description
 input should be a P^{2 (not a list)  so then also fix 18591863 }
 comment on why you have 'and' in the is_square
 citation (Hutz  Thesis)
 case 4.2.2 is missing in 19151918
 1923 remove comment
 combine checking for duplicates with detereming the fiber (i.e., check if in Fibers before adding
 rename Fibers to fiber
 1940 remove this comment
orbit_phi:
 remove 1947
 input needs a blank line after and has a terminating white space that should be removed
 fix description of N (see projective_point.orbit)
 another example where N is a list [m,n]
 can't always normalize such as in CC. Should you add a keyword as is projective_point.orbit?
orbit_psi:
 all the same comments
is_isomorphic:
 add reasoning to the description
 add a False example
 fix example spacing
is_symmetric_orbit:
 blank line befor function
 better function description
 fixing spacing 20752076
 this could be given a list of random points. You need to also check the each succesive is the image of the previous one
 actually you just be given a random list and the function would still run...
 no camelcase for name
 make the name more descriptive of the output
 desribe algorithm (citation)
 actually this is just straight up enumeration
 add a TODO to use the more advanced counting (citation)
 2134  less indent
 2137 needs another space around +
comment:9 Changed 2 years ago by
 Status changed from needs_review to needs_work
Here is the last bit, reviewing the degenerate fiber morphisms:
sigamX:
 103435, 104748, 106162: are all exactly the same, so can be before the ifs
 1036, 1037  switch order so that t = w1t1 (and in the other 2 places)
 put the T after the ifs since they are all the same
 1078: use .divides
 10761088: needs some comments
 1116 use divides
 The whole degenerate fiber computation is badly in need of comments/explanation
all the same comments for sigmaY
comment:10 Changed 2 years ago by
 Commit changed from 9b268f823c0e0ff35f0b881dadaa50ca72bfe875 to b987250835ecba1acc43c020b8f4d5f314adfaf8
Branch pushed to git repo; I updated commit sha1. New commits:
b987250  17282 Fixed issues laid out in review

comment:11 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:12 Changed 2 years ago by
 Status changed from needs_review to needs_work
There are a couple issues from the above lists that were not fixed and there are a few more things I found:
 is_isomorphic()  other > right
 sigmax()  P2 x 'P2 should not have '
 Lx*, Qx*, Sx*  need to check that a is a point in P2
 it would be better to check that it is the 'correct' projective space, self.ambient_space()[*]
 Ramification_poly() definition: yes that is the formula for it, but what is it?
 degenerate_primes: Fix the formatting for the default value. (True default) > Default: True
 check for other instances in the file
 some comments in lambda_plus, lambda_minus wouldn't hurt
 trailing white space in fiber()
 nth_iterate_phi, nth_iterate_psi
 p > P
 n an integer (not a positive integer)
 is_symmetric_orbit: you don't just want a list of points, you want a periodic cycle...
 normalize parameter for phi, psi, sigmaX, sigmaY, orbit_phi, orbit_psi, nth_iterate_phi, nth_iterate_psi
 use self.normalize_coordinates() to normalize (inherited from projective product)
 when these functions are called and the base_ring is a field it should normalize in the same way init does (make the last nonzero coordinate 1)
PP.<x0,x1,x2,y0,y1,y2> = ProductProjectiveSpaces([2,2],GF(3)) L = x0*y0 + x1*y1  x2*y2 Q = x0*x1*y1^2 + x2^2*y0*y2 W = WehlerK3Surface([L,Q]) H=End(W) f=H([x0,x1,x2,y0,y1,y2])
 the _morphism() function is not needed since it will inherit it from its ambient space (projective product)
 The coordinates of the points need to be coerced into the base_ring
PP.<x0,x1,x2,y0,y1,y2> = ProductProjectiveSpaces([2,2],GF(3)) L = x0*y0 + x1*y1  x2*y2 Q = x0*x1*y1^2 + x2^2*y0*y2 W = WehlerK3Surface([L,Q]) P=W.point([0,2,0,2,0,0],False);P P.normalize_coordinates();P
This may actually be an issue from projective products.
 I have not yet run the docbuild, but I noticed a number of typos in the documentation, you should probably take a close look at the html version to catch those.
comment:13 Changed 2 years ago by
 Commit changed from b987250835ecba1acc43c020b8f4d5f314adfaf8 to 26bff005e05b2a151baceb1134d9c50d8f160ec2
comment:14 Changed 2 years ago by
 Commit changed from 26bff005e05b2a151baceb1134d9c50d8f160ec2 to 092c8cd9095248afc60cda1382b920d9ace99688
Branch pushed to git repo; I updated commit sha1. New commits:
092c8cd  17282 Adjusted problems from last review

comment:15 Changed 2 years ago by
 Commit changed from 092c8cd9095248afc60cda1382b920d9ace99688 to 33c3109a3856aa468da5d2fb0fc14aaf9b62283e
Branch pushed to git repo; I updated commit sha1. New commits:
33c3109  17282 Fixed documentation issues and commented code

comment:16 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:17 Changed 2 years ago by
 Milestone changed from sage6.6 to sage6.7
 Status changed from needs_review to needs_work
Getting closer so I did a more thorough functionality testing:
 line 2224 note that it is an example of an "asymmetric 8cycle with points on degenerate fibers"
 degenerate_fibers seems to be failing for Qp
R.<x0,x1,x2,y0,y1,y2>=!PolynomialRing(Qp(3, 300),6) Y=x0*y0+x1*y1x2*y2 Z=x0!^2*y0*y1 + x0!^2*y2!^2  x0*x1*y1*y2 + x1!^2*y2*y1 +x2!^2*y2!^2 + x2!^2*y1!^2 +x1!^2*y2!^2 X=WehlerK3Surface([Z,Y]) print X.is_degenerate() X.degenerate_fibers()
 Here is another interesting example you can include
R.<x0,x1,x2,y0,y1,y2>=!PolynomialRing(QQ,6) L= (y0  y1)*x0 + (y0*x1  y2*x2) Q=(y2*y0  y1!^2)*x0!^2 + ((y0!^2  y2*y0 + (y2*y1  y2!^2))*x1 + (y0!^2  y2*y1)*x2)*x0 + ((y0!^2  y2*y0  y2!^2)*x1!^2 + (y2*y0  y1!^2)*x2*x1 + (y0!^2 + (y1  y2)*y0)*x2!^2) X=WehlerK3Surface([L,Q]) P=X([1,0,1,1,1,0]) #order 16 X.nth_iterate_phi(P,8) == X.nth_iterate_psi(P,8)
 here is a failure in sigmaY (degenerate)
PP.<x0,x1,x2,y0,y1,y2>=!ProductProjectiveSpaces([2,2],GF(3)) l=x0*y0 + x1*y1 + x2*y2 q=3*x0!^2*y0!^2 + 4*x0*x1*y0!^2  3*x0*x2*y0!^2  5*x0!^2*y0*y1  190*x0*x1*y0*y1  5*x1!^2*y0*y1 + 5*x0*x2*y0*y1 + 14*x1*x2*y0*y1 + 5*x2!^2*y0*y1  x0!^2*y1!^2  6*x0*x1*y1!^2  2*x1!^2*y1!^2 + 2*x0*x2*y1!^2  4*x2!^2*y1!^2 + 4*x0!^2*y0*y2  x1!^2*y0*y2 + 3*x0*x2*y0*y2 + 6*x1*x2*y0*y2  6*x0!^2*y1*y2  4*x0*x1*y1*y2  x1!^2*y1*y2 + 51*x0*x2*y1*y2  7*x1*x2*y1*y2  9*x2!^2*y1*y2  x0!^2*y2!^2  4*x0*x1*y2!^2 + 4*x1!^2*y2!^2  x0*x2*y2!^2 + 13*x1*x2*y2!^2  x2!^2*y2!^2 X=WehlerK3Surface([l,q]) print X.degenerate_fibers() P=X([0,1,1,1,0,0]) print P print X.sigmaY(P) X.sigmaY(X.sigmaY(P))
 degenerate primes should return NotImplementedError? instead of TypeError?
TypeError: Must be ZZ or QQ
 you have a couple long test failures due to a lack of
set_verbose(None)
comment:18 Changed 2 years ago by
 Commit changed from 33c3109a3856aa468da5d2fb0fc14aaf9b62283e to 1edfe71b0fda3685fd3edc0bd9b9e1532d549f28
Branch pushed to git repo; I updated commit sha1. New commits:
1edfe71  17282 Fixed the issues found in last pass

comment:19 Changed 2 years ago by
 Status changed from needs_work to needs_review
I've gone through and fixed the sigmaY issue as well as the long test failures, unfortunately padic factoring for polynomials is not working properly now, so that error is unavoidable, setting to needs review once more
comment:20 Changed 2 years ago by
 Status changed from needs_review to needs_work
Here is what I see in the documentation:
ramification poly
 cite call silverman
 output: polynomial in coordinate ring of ambient space
canonical height
 change '' to 'minus'
canonical height minus
 change '+' to 'minus'
 'Use v=0 for the archimedean place' should be deleted
 add (local heights) after 'lambda minus heights'
canonical height plus
  'Use v=0 for the archimedean place' should be deleted
 add (local heights) after 'lambda plus heights'
degenerate fibers
 Remove the extra 'in' in 'The output is a list of lists where in the elements of lists are points in the appropriate projective space.'
degenerate primes
 only over ZZ or QQ
is_symmetric_oribt
 fix 'ps`'
lambda minus
 change '' to 'minus'
lambda plus
 change '+' to 'plus'
nth_iterate_phi
 write out 'phi'
 quotes for `psi' is fine, but you should be consistent throughout the
documentation. This is thefirst place you've done that.
nth_iterate_phi
 write out 'psi'
orbit_phi
 Function is defined in [CaSi]
phi
 output: a point on self
psi
 output: a point on self
sigmaX
 ``self``  you have single quotes
 change 'while we have two points in the fiber' to 'while we can split the fiber into pairs of points'
 output: a point on self
sigmaY
 ``self``  you have single quotes
 change 'while we have two points in the fiber' to 'while we can split the fiber into pairs of points'
 output: a point on self
you are inconsistent with the hyphenation of bilinear and biquadratic. I think it
should not be hyphenated.
For functionality
fiber:
 add a component = 0 example
 shouldn't the last 3 cases have an else for component == 1?
PP.<x0,x1,x2,y0,y1,y2> = ProductProjectiveSpaces([2,2],GF(7)) L = x0*y0 + x1*y1  1*x2*y2 Q=(2*x0^2 + x2*x0 + (2*x1^2 + x2^2))*y0^2 + ((x0^2 + x1*x0 +(x1^2 + 2*x2*x1 + x2^2))*y1 + (2*x1^2 + x2*x1 + x2^2)*y2)*y0 + ((2*x0^2+ (x1 + 2*x2)*x0 + (2*x1^2 + x2*x1))*y1^2 + ((2*x1 + 2*x2)*x0 + (x1^2 +x2*x1 + 2*x2^2))*y2*y1 + (2*x0^2 + x1*x0 + (2*x1^2 + x2^2))*y2^2) W = WehlerK3Surface([L,Q]) W.fiber([4,0,1],0)
Should have 2 points...
PP.<x0,x1,x2,y0,y1,y2> = ProductProjectiveSpaces([2,2],GF(7)) L = x0*y0 + x1*y1  1*x2*y2 Q = (2*y0^2 + y2*y0 + (2*y1^2 + y2^2))*x0^2 + ((y0^2 + y1*y0 +(y1^2 + 2*y2*y1 + y2^2))*x1 + (2*y1^2 + y2*y1 + y2^2)*x2)*x0 + ((2*y0^2+ (y1 + 2*y2)*y0 + (2*y1^2 + y2*y1))*x1^2 + ((2*y1 + 2*y2)*y0 + (y1^2 +y2*y1 + 2*y2^2))*x2*x1 + (2*y0^2 + y1*y0 + (2*y1^2 + y2^2))*x2^2) W = WehlerK3Surface([L,Q]) W.fiber([1,0,0],1)
This is returning a point over base field with a entry in a quadratic extension. That is 2 issues.
nth_iterates
 Unless I'm missing something, the negative seems not to be working
R.<x0,x1,x2,y0,y1,y2>=PolynomialRing(QQ,6) L = (y0  y1)*x0 + (y0*x1  y2*x2) Q = (y2*y0  y1^2)*x0^2 + ((y0^2  y2*y0 + (y2*y1  y2^2))*x1 + (y0^2  y2*y1)*x2)*x0 + ((y0^2  y2*y0  y2^2)*x1^2 + (y2*y0  y1^2)*x2*x1 + (y0^2 + (y1  y2)*y0)*x2^2) X = WehlerK3Surface([L,Q]) P = X([1,0,1,1,1,0]) X.nth_iterate_phi(P,1)==X.nth_iterate_psi(P,1)
comment:21 Changed 2 years ago by
 Commit changed from 1edfe71b0fda3685fd3edc0bd9b9e1532d549f28 to 728d278b2d2696a7e68a4cbc05ff1f2b323d96f0
Branch pushed to git repo; I updated commit sha1. New commits:
728d278  17282 fixed functionality and documentation issues

comment:22 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:23 Changed 2 years ago by
 Commit changed from 728d278b2d2696a7e68a4cbc05ff1f2b323d96f0 to 97700cb6b9ddb04dc1c03b115246989f6adce38f
Branch pushed to git repo; I updated commit sha1. New commits:
97700cb  17262 Fixed minor typos in documentation

comment:24 Changed 2 years ago by
 Status changed from needs_review to positive_review
looks good now.
comment:25 Changed 2 years ago by
 Status changed from positive_review to needs_work
PDF docs don't build
comment:26 Changed 2 years ago by
 Milestone changed from sage6.7 to sage6.8
 Status changed from needs_work to positive_review
I'm getting the same 82 warnings when building the reference/scheme folder as pdf on 6.7 with and without this ticket included. So the failure seems to have nothing to do with this ticket.
fyi, they are all of the form:
sage/sagedev/src/doc/en/reference/schemes/sage/schemes/toric/weierstrass.rst:: WARNING: unusable reference target found: ../../../../../../../html/en/reference/geometry/sage/geometry/lattice_polytope.html#sage.geometry.lattice_polytope.ReflexivePolytopes
comment:27 Changed 2 years ago by
 Status changed from positive_review to needs_work
I don't know what you are doing wrong but underscore is definitely not valid it TeX text mode:
! Missing $ inserted. <inserted text> $ l.28196 \bibitem[Hutz_thesis]{Hutz_thesis} {\phantomsection\label{sage/scheme... ? [164]] ! Emergency stop.
comment:28 Changed 2 years ago by
 Commit changed from 97700cb6b9ddb04dc1c03b115246989f6adce38f to 930cbb023cdf849c85acf1dc67cc32274ccbe7fa
comment:29 Changed 2 years ago by
I merged the ticket in with 6.7, and fixed the name of the reference to take out the underscore and now I come back with only three errors:
[schemes ] None:None: WARNING: citation not found: Fulton [schemes ] /home/joao/sagedev/src/doc/en/reference/schemes/index.rst:: WARNING: unusable reference target found: ../genindex.html [schemes ] /home/joao/sagedev/src/doc/en/reference/schemes/index.rst:: WARNING: unusable reference target found: ../pymodindex.html [schemes ] /home/joao/sagedev/src/doc/en/reference/schemes/index.rst:: WARNING: unusable reference target found: ../search.html
however, it still fails to build.
comment:30 Changed 2 years ago by
 Status changed from needs_work to needs_review
Per sagedevel, installed tex and got the issue to resolve, docs built successfully on my machine
comment:31 Changed 2 years ago by
 Status changed from needs_review to needs_work
I fixed my tex installation and this builds now for me as well. However, the first line of the file is used as the section heading and it is currently "AUTHORS". Change that to something meaningful like "Wehler K3 surfaces"
comment:32 Changed 2 years ago by
 Commit changed from 930cbb023cdf849c85acf1dc67cc32274ccbe7fa to 08877a497294020d4b090daba982560fe2b5b4be
Branch pushed to git repo; I updated commit sha1. New commits:
08877a4  17282 fixed title

comment:33 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:34 Changed 2 years ago by
 Status changed from needs_review to positive_review
New commits:
08877a4  17282 fixed title

comment:35 Changed 2 years ago by
 Branch changed from u/jdefaria/ticket/17282 to 08877a497294020d4b090daba982560fe2b5b4be
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Added Wk3 file