Opened 9 years ago
Closed 8 years ago
#15808 closed enhancement (fixed)
Remove genus2reduction
Reported by:  Jeroen Demeyer  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  packages: standard  Keywords:  
Cc:  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Peter Bruin 
Report Upstream:  N/A  Work issues:  
Branch:  f35c944 (Commits, GitHub, GitLab)  Commit:  f35c94473e316c01c968f2a43027868c517815a2 
Dependencies:  #15767  Stopgaps: 
Description (last modified by )
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)
Change History (24)
comment:1 Changed 9 years ago by
Description:  modified (diff) 

comment:2 Changed 8 years ago by
Milestone:  sage6.2 → sage6.3 

comment:3 Changed 8 years ago by
Milestone:  sage6.3 → sage6.4 

comment:4 Changed 8 years ago by
Branch:  → u/jdemeyer/ticket/15808 

Created:  Feb 11, 2014, 9:55:16 AM → Feb 11, 2014, 9:55:16 AM 
Modified:  Aug 10, 2014, 4:51:03 PM → Aug 10, 2014, 4:51:03 PM 
comment:5 Changed 8 years ago by
Commit:  → 927e70cfb6eb1a51d119351ab1035504bd4ecb26 

comment:6 Changed 8 years ago by
Authors:  → Jeroen Demeyer 

Status:  new → needs_review 
comment:7 Changed 8 years ago by
Commit:  927e70cfb6eb1a51d119351ab1035504bd4ecb26 → 701a3dfc54729ce1c0467688b711cbf33d2eeca2 

Branch pushed to git repo; I updated commit sha1. New commits:
701a3df  Upgrade to PARI git master

comment:8 Changed 8 years ago by
Commit:  701a3dfc54729ce1c0467688b711cbf33d2eeca2 → 3ffdf18b24d4b8beb8976e6407d3a5cddb0efba5 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
3ffdf18  Upgrade to PARI git master

comment:9 Changed 8 years ago by
Commit:  3ffdf18b24d4b8beb8976e6407d3a5cddb0efba5 → 927e70cfb6eb1a51d119351ab1035504bd4ecb26 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:11 Changed 8 years ago by
Commit:  927e70cfb6eb1a51d119351ab1035504bd4ecb26 → 11946d65c963f03b83d0c03503ccad16377f4e46 

Branch pushed to git repo; I updated commit sha1. New commits:
11946d6  Fix documentation

comment:12 Changed 8 years ago by
Branch:  u/jdemeyer/ticket/15808 → public/ticket/15808 

Commit:  11946d65c963f03b83d0c03503ccad16377f4e46 → 1068fef87401f820747d4f938a56d2429afbe4b0 
comment:13 followup: 14 Changed 8 years ago by
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*(d1)  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 Changed 8 years ago by
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*(d1))
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^(d1)
.
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 8 years ago by
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): 514 514 minimal_disc = QQ(res[2].poldisc()).abs() 515 515 if minimal_equation.degree() == 5: 516 516 minimal_disc *= minimal_equation[5]**2 517 # Multiply with suitable power of 2 of the form 2^(2* (d1) 12)517 # Multiply with suitable power of 2 of the form 2^(2*d  12) 518 518 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 520 520 minimal_disc >>= 12  b*k 521 521 minimal_disc = ZZ(minimal_disc) 522 522
This does not affect any doctests in genus2reduction.py
.
comment:16 followup: 19 Changed 8 years ago by
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 2adic 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 8 years ago by
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 2part 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 8 years ago by
Attachment:  discriminant.patch added 

comment:19 followup: 22 Changed 8 years ago by
Replying to pbruin:
However, your formula seems to mean that the 2adic 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 8 years ago by
Branch:  public/ticket/15808 → u/jdemeyer/ticket/15808 

Modified:  Oct 8, 2014, 1:37:09 PM → Oct 8, 2014, 1:37:09 PM 
comment:21 Changed 8 years ago by
Commit:  1068fef87401f820747d4f938a56d2429afbe4b0 → f35c94473e316c01c968f2a43027868c517815a2 

New commits:
f35c944  Fix "away from 2" check

comment:22 Changed 8 years ago by
Reviewers:  → Peter Bruin 

Status:  needs_review → positive_review 
Replying to jdemeyer:
Replying to pbruin:
However, your formula seems to mean that the 2adic 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 8 years ago by
Branch:  u/jdemeyer/ticket/15808 → f35c94473e316c01c968f2a43027868c517815a2 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Remove genus2reduction