Opened 12 years ago
Closed 12 years ago
#8997 closed defect (fixed)
riemann_roch_basis is implemented incorrectly in sage
Reported by: | was | Owned by: | AlexGhitza |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6.2 |
Component: | algebraic geometry | Keywords: | |
Cc: | rkirov, minz, OleksandrMotsak | Merged in: | sage-4.6.2.alpha1 |
Authors: | Moritz Minzlaff | Reviewers: | David Joyner, Oleksandr Motsak |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
See the file schemes/plane_curves/projective_curve.py, where it says
The following example illustrates that the Riemann-Roch space function in Singular doesn't *not* work correctly. :: sage: R.<x,y,z> = GF(5)[] sage: f = x^7 + y^7 + z^7 sage: C = Curve(f); pts = C.rational_points() sage: D = C.divisor([ (3, pts[0]), (-1,pts[1]), (10, pts[5]) ]) sage: C.riemann_roch_basis(D) # output is random (!!!!) [x/(y + x), (z + y)/(y + x)] The answer has dimension 2 (confirmed via Magma). But it varies between 1 and quite large with Singular.
The problem can be solved by learning how the relevant code in Singular works then correctly wrapping it.
Attachments (2)
Change History (24)
comment:1 Changed 12 years ago by
- Component changed from algebra to algebraic geometry
- Milestone changed from sage-5.0 to sage-4.4.3
comment:2 in reply to: ↑ description Changed 12 years ago by
comment:3 follow-up: ↓ 4 Changed 12 years ago by
For the record, the relevant Singular ticket is here: http://www.singular.uni-kl.de:8002/trac/ticket/153
comment:4 in reply to: ↑ 3 Changed 12 years ago by
Replying to was:
For the record, the relevant Singular ticket is here: http://www.singular.uni-kl.de:8002/trac/ticket/153
Thanks. This suggests that all one needs to do is to add the singular command
system("random", mySeedAsAnInt);
at the top of the function code.
comment:5 Changed 12 years ago by
- Status changed from new to needs_work
The attached patch seems to fix the problem. When applied to 4.4.2 on a 10.6.3 mac, it passes sage -testall except for an unrelated docstring failure (in interfaces/r.py).
I'm leaving it as needs work for now since I want to test it on more machines. I'm posting it in case others want to test it too.
comment:6 Changed 12 years ago by
This patch does not work. My guess is that
system("random", mySeedAsAnInt);
does not really set the random seed for all commands, but I could easily be wrong. In any case, it seems that the command now does return the dimension in a consistent way for different machines. That is progress, since the old version was much worse. However, the basis (ie, the list of functions spanning the RR space) is not deterministic. I'm not sure how to fix that.
comment:7 Changed 12 years ago by
- Cc rkirov added
comment:8 Changed 12 years ago by
- Cc minz added
comment:9 Changed 12 years ago by
My impression is that the problem lies with Singular. I adapted the example in the description above and typed directly into Singular the following:
kill s, C, Ctemp, temp, G, R, LG; LIB "brnoeth.lib"; int plevel=printlevel; printlevel=-1; system("random", 1); ring s=5,(x,y),lp; list C=Adj_div(x7+y7+1); C=NSplaces(1,C); def R=C[1][2]; # I want to look at the points to be able to define # the same divisor each time, see below def Ctemp=extcurve(1,C); def temp=Ctemp[1][5]; setring temp; print(POINTS); setring R; # adapt the line below according to the ordering of the points # i always chose the divisor 3(0,-1,1)-1(1,2,1)+10(2,1,1) intvec G = ; list LG=BrillNoether(G,C); LG; printlevel=plevel;
Not only did the bases vary each time I ran this code (even though I fixed the random seed in the sixth line), the resulting bases also had different cardinality (either 0 or 2).
(I also tried to post this on the Singular trac server, but failed to do so. Maybe someone else is able to update the Singular ticket?)
comment:10 Changed 12 years ago by
- Status changed from needs_work to needs_info
Dear minz,
- what do you mean with "intvec G = ;" in the Singular code?
- could you please answer to the comment by Jose Ignacio Farran at
http://www.singular.uni-kl.de:8002/trac/ticket/153#comment:7 ?
comment:11 Changed 12 years ago by
- Cc OleksandrMotsak added
comment:12 follow-up: ↓ 13 Changed 12 years ago by
- Status changed from needs_info to needs_review
Following Jose's explanations on the Singular trac server, the modified Sage wrapper should now work with the new patch. What the unmodified wrapper did wrong was how it defined the divisor in Singular. There's actually two lists that are important: The divisor needs to be defined relative to the list of points C[3] (in the above example), but to know which point the k-th entry of this list actually refers to, one has to parse the list POINTS of the rings C[5][d][1], where d is the degree of the point. -- The patch also modifies the documentation to illustrate this with an example.
comment:13 in reply to: ↑ 12 Changed 12 years ago by
Replying to minz:
Following Jose's explanations on the Singular trac server, the modified Sage wrapper
...
Thank you!
I'll look at this carefully when classes end this semester, which will be in a few weeks.
comment:14 Changed 12 years ago by
- Status changed from needs_review to positive_review
This looks good. It applies fine to sage 4.6 on an ubuntu linux machine and passes sage -testall. I have fixed the code and docstrings in agcode.py so that it runs too. I guess this should be a separate ticket?
Again, thanks *very* much!
comment:15 Changed 12 years ago by
- Milestone changed from sage-4.6.1 to sage-4.6.2
comment:16 follow-up: ↓ 17 Changed 12 years ago by
- Status changed from positive_review to needs_info
This needs some clarifications:
- which patch(es) need to be applied?
- who are the authors/reviewers? (please fill in the real names in the Author and Reviewer fields on this ticket)
comment:17 in reply to: ↑ 16 ; follow-up: ↓ 18 Changed 12 years ago by
- Reviewers set to David Joyner, Oleksandr Motsak
Replying to jdemeyer:
This needs some clarifications:
- which patch(es) need to be applied?
- who are the authors/reviewers? (please fill in the real names in
the Author and Reviewer fields on this ticket)
Done. Others helped, such as William Stein and Jose Farran.
Many thanks to everyone who helped with fixing this issue!
Can this be changed back to positive review now?
comment:18 in reply to: ↑ 17 ; follow-up: ↓ 19 Changed 12 years ago by
comment:19 in reply to: ↑ 18 Changed 12 years ago by
comment:20 Changed 12 years ago by
- Status changed from needs_info to needs_review
comment:21 Changed 12 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
comment:22 Changed 12 years ago by
- Merged in set to sage-4.6.2.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
Replying to was:
Are there any more details on this available?