Opened 11 years ago

Closed 11 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:

Status badges

Description (last modified by jdemeyer)

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.

Apply: trac_8997_fix_rr_basis_and_doc.patch

Attachments (2)

trac_8997-riemann-roch.patch (6.9 KB) - added by wdj 11 years ago.
apply to 4.4.2
trac_8997_fix_rr_basis_and_doc.patch (10.2 KB) - added by minz 11 years ago.
apply to 4.6

Download all attachments as: .zip

Change History (24)

comment:1 Changed 11 years ago by was

  • 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 11 years ago by wdj

Replying to was:

The problem can be solved by learning how the relevant code in Singular works then correctly wrapping it.

Are there any more details on this available?

comment:3 follow-up: Changed 11 years ago by was

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 11 years ago by wdj

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.

Changed 11 years ago by wdj

apply to 4.4.2

comment:5 Changed 11 years ago by wdj

  • 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 11 years ago by wdj

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 11 years ago by rkirov

  • Cc rkirov added

comment:8 Changed 11 years ago by minz

  • Cc minz added

comment:9 Changed 11 years ago by minz

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 11 years ago by OleksandrMotsak

  • Status changed from needs_work to needs_info

Dear minz,

  1. what do you mean with "intvec G = ;" in the Singular code?
  2. 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 11 years ago by OleksandrMotsak

  • Cc OleksandrMotsak added

Changed 11 years ago by minz

apply to 4.6

comment:12 follow-up: Changed 11 years ago by minz

  • 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 11 years ago by wdj

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 11 years ago by wdj

  • 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 11 years ago by jdemeyer

  • Milestone changed from sage-4.6.1 to sage-4.6.2

comment:16 follow-up: Changed 11 years ago by jdemeyer

  • 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: Changed 11 years ago by wdj

  • Authors set to Moritz Minzlaff
  • 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: Changed 11 years ago by jdemeyer

Replying to wdj:

Replying to jdemeyer:

This needs some clarifications:

  • which patch(es) need to be applied?

You didn't answer this question...

comment:19 in reply to: ↑ 18 Changed 11 years ago by wdj

Replying to jdemeyer:

Replying to wdj:

Replying to jdemeyer:

This needs some clarifications:

  • which patch(es) need to be applied?

You didn't answer this question...

Sorry! Just trac_8997_fix_rr_basis_and_doc.patch

comment:20 Changed 11 years ago by jdemeyer

  • Status changed from needs_info to needs_review

comment:21 Changed 11 years ago by jdemeyer

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

comment:22 Changed 11 years ago by jdemeyer

  • Merged in set to sage-4.6.2.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.