edgarcosta / endomorphisms

Rigorous computation of the endomorphism ring of a Jacobian
GNU General Public License v2.0
10 stars 8 forks source link

SE_Curve #6

Closed jvoight closed 5 years ago

jvoight commented 6 years ago

I downloaded a fresh ZIP file and ran this in the magma directory, presumably an easy fix, but I actually can't find the function SE_Curve.

> AttachSpec("spec");
> _<x> := PolynomialRing(Rationals());
> X := HyperellipticCurve([x^5, x^5+1]);
> prec := 300;      
> eqsCC := EmbedCurveEquations(X, prec);
> eqsF := DefiningEquations(X);
> P := PeriodMatrix(eqsCC, eqsF : MolinNeurohr := false);

In file "/Users/jvoight/Downloads/endomorphisms-master/endomorphisms/magma/heuristic/Periods.m", line 33, column 10:
>>     X := SE_Curve(gCC, 2 : Prec := Precision(CC));
            ^
Runtime error: Undefined reference 'SE_Curve' in package 
"/Users/jvoight/Downloads/endomorphisms-master/endomorphisms/magma/heuristic/Periods.m"
edgarcosta commented 6 years ago

@jvoight, could you try again? I believe I have just fixed it with an horrible workaround. I tried your example on my side and now it seems to work.

@michaelmusty, do you know of a better workaround to avoid preparsing errors of things that aren't going to be called? In short, SE_curve might (or not) be defined in an external package, but since magma is interpreted, it shouldn't stop us from mentioning it. See, https://github.com/edgarcosta/endomorphisms/blob/master/endomorphisms/magma/heuristic/Periods.m#L29

JRSijsling commented 6 years ago

Sorry, this is not quite made clear in the readme, but you have to install the package by Molin and Neurohr mentioned there and add that to your .magmarc.

edgarcosta commented 6 years ago

@JRSijsling, let me play devils advocate, what if I don't want to install the package by Molin and Neurohr?

edgarcosta commented 6 years ago

Alternatively, we can also add a "symlink" to their package.

https://blog.github.com/2016-02-01-working-with-submodules/

jvoight commented 6 years ago

Thanks Edgar, the hack works for me, and I don't know of an alternative.

I'm wary of creating a unnecessary dependency on another package. In my situation, I can make my own period matrices, and although I don't mind installing something else, I don't feel like I should have to. And what happens if and when they update their package, possibly breaking things for us?

JRSijsling commented 6 years ago

Yeah, I see the point of not relying on other packages. But here it is really better than the standard Magma alternative. And one can always rewind the checkout of the other package if things break.

The error is due to my lack of knowledge. Magma tries to parse all bits of the code, regardless of whether it is ever run. So even if you set MolinNeurohr to false it tries to find the corresponding functions. I do not know how to fix that.

jvoight commented 6 years ago

Yes, I also understand that currently the other package is superior to Magma. But:

The user of our package may not want to or be expert enough to keep track of an extra package, or to rewind a checkout.

Anyway, the issue is fixed, so we can close this thread. I'm not meaning to be bossy, just trying to save us a headache down the road.

JRSijsling commented 6 years ago

Yes, thanks for thinking with us! The submodule thing might be a good idea to solve this appropriately, because we cannot get around using the other package in some form. Indeed user-friendliness is to be considered.

In the meantime, here is how one can react to changes with the current code:

If Magma ever gets a better standard algorithm, just leave the package and set MolinNeurohr to false.

Starting with the period matrix is possible, as shown by examples/Single.m; just insert your own on line 80. In Sage this is more difficult, but what I do is to cheat; I just make a curve of the correct genus and after initialization I modify the period matrix attribute before calling anything else.

edgarcosta commented 6 years ago

I think it makes more sense to think of MolinNeurohr as an addon, instead of a dependency, as we only use it for the period matrix, and even then is optional for most cases.

If we all agreed with this, I would close the issue and revisit this if we ever need to.

JRSijsling commented 6 years ago

Is this fixed in the current version of the project? For me things go wrong, now with

In file "/Users/jrsijsling/Programs/sage/local/lib/python2.7/site-packages/endomorphisms/magma/heuristic/Periods.m", line 41, column 14:
>>         X := SE_Curve(gCC, 2 : Prec := Precision(CC));
                ^
Runtime error: Variable 'SE_Curve' has not been initialized

This happens when running either of Single.m and Single.sage.

michaelmusty commented 6 years ago

I was having this problem too. If you attach the MolinNeurohr (https://github.com/pascalmolin/hcperiods) spec file, then the superelliptic functions become available in the session, but the error still occurs if you want to use it in another function. For example, if we start in the endomorphisms example directory, then the code

AttachSpec("../../hcperiods/magma/spec"); AttachSpec("../endomorphisms/magma/spec"); R<x,y> := PolynomialRing(Rationals(), 2); f := y^3+x^5-1; X := PlaneCurve(f); eqsCC := EmbedCurveEquations(X, 300); eqsK := DefiningEquations(X); test, fCC, e := IsSuperelliptic(eqsCC); assert test; X := SE_Curve(fCC, 3 : Prec := 300); P := X`BigPeriodMatrix; // P := PeriodMatrix(eqsCC, eqsK : MolinNeurohr := true);

works fine, but executing the commented line (which calls SE_Curve) brings about this error. This seems pretty strange since all I did to get the lines above to run was pluck them out of the PeriodMatrix intrinsic.

michaelmusty commented 6 years ago

Edgar and I will continue to try to fix this, but if not then we will have to include hcperiods as a submodule.

JRSijsling commented 6 years ago

How is this coming along? Iti is a bit frustrating that the current master branch is something for which the examples fail to work.

edgarcosta commented 6 years ago

Howdy,

Mike and I didn't manage to find a workaround it. Jeroen had tried in the past and was also unsuccessful. I propose we include MolinNeurohr as a submodule. This will be a temporary solution as I'm aware that Chris is trying to get the period package integrated into Magma.

Jeroen, on my way to Wales, I will fix once I'm on that side of the pond. (If you prefer you can go ahead an revert my broken fix)

On Mon, Aug 6, 2018, 09:38 Jeroen Sijsling notifications@github.com wrote:

How is this coming along? Iti is a bit frustrating that the current master branch is something for which the examples fail to work.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/edgarcosta/endomorphisms/issues/6#issuecomment-410710809, or mute the thread https://github.com/notifications/unsubscribe-auth/AATtBiX96tNSvVKeVX-_4ACW812xpnzRks5uOEbQgaJpZM4SluUL .

JRSijsling commented 6 years ago

Right now the README.md tells the users to install hcperiods. In my opinion this is not a bad solution. The user can be expected to put in that minor effort. Moreover, I feel that integrating hcperiods does not emphasize Molin--Neurohr's work enough. And at any rate, once this is part of Magma, then we can switch automatically.

JRSijsling commented 5 years ago

Update: both that work and the algorithms for general curves will soon be part of Magma. So I suggest to leave this aside until that time, after which it will be straightforward to integrate.

We should still give a clear indication and attribution of their work, since it plays such an indispensable role in the heuristics.

JRSijsling commented 5 years ago

Both required algorithms (calculation of superelliptic and calculation of plane period matrices) are now available in dependencies indicated in the README. The user can install these him- or herself.

For me this means that this is no longer an issue. However, I will not close it, as I know that some of you prefer installing dependencies as submodules.

edgarcosta commented 5 years ago

Given that these will dependencies will soon be available in Magma, I say we are done for now, and it is not worth the effort of maintaining submodules.