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, 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 7 years ago by
- Description modified (diff)
comment:2 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:3 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:4 Changed 6 years ago by
- 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
- Commit set to 927e70cfb6eb1a51d119351ab1035504bd4ecb26
comment:6 Changed 6 years ago by
- Status changed from new to needs_review
comment:7 Changed 6 years ago by
- Commit changed from 927e70cfb6eb1a51d119351ab1035504bd4ecb26 to 701a3dfc54729ce1c0467688b711cbf33d2eeca2
Branch pushed to git repo; I updated commit sha1. New commits:
701a3df | Upgrade to PARI git master
|
comment:8 Changed 6 years ago by
- Commit changed from 701a3dfc54729ce1c0467688b711cbf33d2eeca2 to 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 6 years ago by
- 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
There is an EXAMPLES:
missing a double ::
comment:11 Changed 6 years ago by
- Commit changed from 927e70cfb6eb1a51d119351ab1035504bd4ecb26 to 11946d65c963f03b83d0c03503ccad16377f4e46
Branch pushed to git repo; I updated commit sha1. New commits:
11946d6 | Fix documentation
|
comment:12 Changed 6 years ago by
- Branch changed from u/jdemeyer/ticket/15808 to public/ticket/15808
- Commit changed from 11946d65c963f03b83d0c03503ccad16377f4e46 to 1068fef87401f820747d4f938a56d2429afbe4b0
comment:13 follow-up: ↓ 14 Changed 6 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*(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
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
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* (d-1)- 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 follow-up: ↓ 19 Changed 6 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 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
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
comment:18 Changed 6 years ago by
What you say (and your patch) makes sense.
comment:19 in reply to: ↑ 16 ; follow-up: ↓ 22 Changed 6 years ago by
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
- 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
- Commit changed from 1068fef87401f820747d4f938a56d2429afbe4b0 to f35c94473e316c01c968f2a43027868c517815a2
New commits:
f35c944 | Fix "away from 2" check
|
comment:22 in reply to: ↑ 19 Changed 6 years ago by
- 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
- Branch changed from u/jdemeyer/ticket/15808 to f35c94473e316c01c968f2a43027868c517815a2
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Remove genus2reduction