Opened 7 years ago

Closed 6 years ago

#15808 closed enhancement (fixed)

Remove genus2reduction

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.4
Component: packages: standard Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Peter Bruin
Report Upstream: N/A Work issues:
Branch: f35c944 (Commits) Commit: f35c94473e316c01c968f2a43027868c517815a2
Dependencies: #15767 Stopgaps:

Description (last modified by jdemeyer)

genus2reduction has been ported to the PARI library and is available as the PARI function genus2red(). We should remove the package and use the new PARI function instead.

Attachments (1)

discriminant.patch (4.3 KB) - added by pbruin 6 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:3 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:4 Changed 6 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/15808
  • Created changed from 02/11/14 09:55:16 to 02/11/14 09:55:16
  • Modified changed from 08/10/14 16:51:03 to 08/10/14 16:51:03

comment:5 Changed 6 years ago by git

  • Commit set to 927e70cfb6eb1a51d119351ab1035504bd4ecb26

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

927e70cRemove genus2reduction

comment:6 Changed 6 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Status changed from new to needs_review

comment:7 Changed 6 years ago by git

  • Commit changed from 927e70cfb6eb1a51d119351ab1035504bd4ecb26 to 701a3dfc54729ce1c0467688b711cbf33d2eeca2

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

701a3dfUpgrade to PARI git master

comment:8 Changed 6 years ago by git

  • Commit changed from 701a3dfc54729ce1c0467688b711cbf33d2eeca2 to 3ffdf18b24d4b8beb8976e6407d3a5cddb0efba5

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3ffdf18Upgrade to PARI git master

comment:9 Changed 6 years ago by git

  • Commit changed from 3ffdf18b24d4b8beb8976e6407d3a5cddb0efba5 to 927e70cfb6eb1a51d119351ab1035504bd4ecb26

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

comment:10 Changed 6 years ago by chapoton

There is an EXAMPLES: missing a double ::

comment:11 Changed 6 years ago by git

  • Commit changed from 927e70cfb6eb1a51d119351ab1035504bd4ecb26 to 11946d65c963f03b83d0c03503ccad16377f4e46

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

11946d6Fix documentation

comment:12 Changed 6 years ago by chapoton

  • Branch changed from u/jdemeyer/ticket/15808 to public/ticket/15808
  • Commit changed from 11946d65c963f03b83d0c03503ccad16377f4e46 to 1068fef87401f820747d4f938a56d2429afbe4b0

There was a pyflakes warning about a missing definition. I made a correction in a function call for that reason.

Also made a few pep8 changes.


New commits:

c4f948dMerge with 6.4.beta4
1068feftrac #15808 correct name for _reduce + a few pep8 changes

comment:13 follow-up: Changed 6 years ago by pbruin

Do you have a reference for the following manipulations?

if minimal_equation.degree() == 5:
    minimal_disc *= minimal_equation[5]**2
# Multiply with suitable power of 2 of the form 2^(2*(d-1) - 12)
b = 2 * (minimal_equation.degree() - 1)
k = QQ((12 - minimal_disc.valuation(2), b)).ceil()
minimal_disc >>= 12 - b*k
minimal_disc = ZZ(minimal_disc)

Is the power of 2 in fact important, given that this is supposed to output the minimal discriminant away from 2?

comment:14 in reply to: ↑ 13 Changed 6 years ago by jdemeyer

Replying to pbruin:

Do you have a reference for the following manipulations?

They are based on looking at the PARI source code. But I must admit that I don't understand everything 100% clearly.

The line

if minimal_equation.degree() == 5:
    minimal_disc *= minimal_equation[5]**2

is based on

  RgX_to_6(polr, &a0,&a1,&a2,&a3,&a4,&a5,&a6);
  I.j10 = !signe(a0)? mulii(sqri(a1), ZX_disc(polr)): ZX_disc(polr);

The factor 2^(-12) comes from

I.j10 = gmul2n(I.j10,-12);

and the multiplication by 2^(2*(d-1)) comes from

dd = polval(polr,gen_2) & (~1); /* = 2 floor(val/2) */
polr = gmul2n(polr, -dd);

which multiplies the whole polynomial by a power of 4. Multiplying a polynomial of degree d by a constant c multiplies the discriminant by c^(d-1).

Is the power of 2 in fact important?

I have no idea. I tried to reproduce as best as possible the output of the old genus2reduction program. That is also the reason why I emulated the raw() method and wrote to divisors_to_string() function: to check for myself if the new output was the same as the old output. Modulo some small details, this seems indeed to be the case.

comment:15 Changed 6 years ago by pbruin

I'm not absolutely sure either, but from the last bit of PARI code you quoted, the following change seems to stay closer to the original:

  • src/sage/interfaces/genus2reduction.py

    diff --git a/src/sage/interfaces/genus2reduction.py b/src/sage/interfaces/genus2reduction.py
    index 73d41d6..01c4019 100644
    a b class Genus2reduction(SageObject): 
    514514        minimal_disc = QQ(res[2].poldisc()).abs()
    515515        if minimal_equation.degree() == 5:
    516516            minimal_disc *= minimal_equation[5]**2
    517         # Multiply with suitable power of 2 of the form 2^(2*(d-1) - 12)
     517        # Multiply with suitable power of 2 of the form 2^(2*d - 12)
    518518        b = 2 * (minimal_equation.degree() - 1)
    519         k = QQ((12 - minimal_disc.valuation(2), b)).ceil()
     519        k = min(a.valuation(2) for a in Q**2 + 4*P) & ~1
    520520        minimal_disc >>= 12 - b*k
    521521        minimal_disc = ZZ(minimal_disc)
    522522

This does not affect any doctests in genus2reduction.py.

comment:16 follow-up: Changed 6 years ago by pbruin

Actually my version above is certainly wrong, because it is not invariant under scaling of the variables in the original equation. However, your formula seems to mean that the 2-adic valuation of the minimal discriminant is bounded on the set of all genus 2 curves, which I can hardly believe, since it isn't even true for elliptic curves.

comment:17 Changed 6 years ago by pbruin

We can also decide to give up trying to imitate the original program perfectly and allow the power of 2 to be different. I am attaching a patch that does this.

I also think you used an incorrect criterion to decide when the 2-part of the conductor was not computed. For example, one of the current doctests has the line

Conductor (away from 2): 954

Note that the "away from 2" part should only be there if the returned conductor is odd. According to the PARI documentation, we should check if 2 appears to the power -1 in the factorisation (fake at 2 in this case) of the conductor. This is fixed by the last part of the patch.

Changed 6 years ago by pbruin

comment:18 Changed 6 years ago by jdemeyer

What you say (and your patch) makes sense.

comment:19 in reply to: ↑ 16 ; follow-up: Changed 6 years ago by jdemeyer

Replying to pbruin:

However, your formula seems to mean that the 2-adic valuation of the minimal discriminant is bounded on the set of all genus 2 curves, which I can hardly believe, since it isn't even true for elliptic curves.

After thinking about this some more, perhaps my code is correct but it depends on the interpretation of "Minimal Discriminant (away from 2)". If you interpret it as "smallest positive integer which occurs as discriminant of a model of the given curve over Z[1/2]", then my code might be correct.

comment:20 Changed 6 years ago by jdemeyer

  • Branch changed from public/ticket/15808 to u/jdemeyer/ticket/15808
  • Modified changed from 10/08/14 13:37:09 to 10/08/14 13:37:09

comment:21 Changed 6 years ago by jdemeyer

  • Commit changed from 1068fef87401f820747d4f938a56d2429afbe4b0 to f35c94473e316c01c968f2a43027868c517815a2

New commits:

f35c944Fix "away from 2" check

comment:22 in reply to: ↑ 19 Changed 6 years ago by pbruin

  • Reviewers set to Peter Bruin
  • Status changed from needs_review to positive_review

Replying to jdemeyer:

Replying to pbruin:

However, your formula seems to mean that the 2-adic valuation of the minimal discriminant is bounded on the set of all genus 2 curves, which I can hardly believe, since it isn't even true for elliptic curves.

After thinking about this some more, perhaps my code is correct but it depends on the interpretation of "Minimal Discriminant (away from 2)". If you interpret it as "smallest positive integer which occurs as discriminant of a model of the given curve over Z[1/2]", then my code might be correct.

Possibly, but I would be surprised if this definition coincides with the one used by the original genus2reduction program. Anyway, I don't think it makes sense to spend too much time on finding a nice definition for "minimal discriminant away from 2", so any power of 2 in the "minimal discriminant away from 2" is fine with me.

comment:23 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/15808 to f35c94473e316c01c968f2a43027868c517815a2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.