#9332 closed enhancement (fixed)
S_class_group() should return a group
Reported by: | stankewicz | Owned by: | David Loeffler |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6.2 |
Component: | number fields | Keywords: | |
Cc: | Alyson Deines, John Cremona, Jeremy West | Merged in: | sage-4.6.2.alpha1 |
Authors: | Jim Stankewicz, Erin Beyerstedt, Anna Haensch, Robert Miller, David Loeffler | Reviewers: | John Cremona |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Attachments (1)
Change History (29)
comment:1 Changed 13 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 13 years ago by
Status: | needs_review → needs_work |
---|
comment:3 Changed 13 years ago by
Authors: | stankewicz → Jim Stankewicz, Erin Beyerstedt, Anna Haensch |
---|
There are still some doctest failures in number_field.py, but we've added documentation and improved the way S_class_groups print. Joint work with Erin Beyerstedt, Robert Miller, H.
comment:4 Changed 13 years ago by
Good work so far!
I think the repr string for a (s-)class group should not mention its order or structure at all. The user can always ask for that.
I'm not sure about the S() function returning None for a straight class group. I would be happy with it returning [] when S is None.
In the number field file, there is no need to add the parameter None for straght class groups, since that is the default value; so take those out.
I will now try the patches out...
comment:5 Changed 13 years ago by
(After testing) When you have simplified the string output of an S-class group you can adjust the doctest outputs in number_field.py. (Don't worry about those Minkowski warnings -- they will disappear when the doctest passes!)
comment:6 Changed 13 years ago by
Description: | modified (diff) |
---|
comment:7 Changed 13 years ago by
For me the 3rd patch does not apply on top of the first two. It also looks strange -- does it mix tabs and spaces? (You should use only spaces, some editors put tabs in).
comment:8 Changed 13 years ago by
Cc: | adeines John Cremona Jeremy West added |
---|
The newest patch is something that I've been working on for the last several days, but I'm making no progress. There's a phantom attribute error which comes up under the following code:
sage: K.<a> = QuadraticField(-14) sage: K.testSclassgroup(tuple([K.ideal(2,a)])) Class group of order 2 with structure C2 of Number Field in a with defining polynomial x^2 + 14 sage: K.testSclassgroup(tuple([K.ideal(2,a)]))(K.ideal(3,a+1)) ERROR: An unexpected error occurred ...
Perhaps there's something stupid I'm missing and someone else can point it out. Otherwise, I'm going to work on something else for a while and come back to this with a fresh head.
comment:9 follow-up: 10 Changed 13 years ago by
The AttributeError? comes up because attributes whose names start with an underscore are private, and ones with two underscores are "very private" -- so private that they aren't even accessible to subclasses. So when code in FractionalIdealClass
sets __ideal
, code in the subclass SFractionalIdealClass
. In fact it is still accessible but under a mangled name: see http://docs.python.org/tutorial/classes.html#private-variables.
But there is another, deeper, bug which will be uncovered when you fix this. After my changes at #9244, a FractionalIdealClass
stores both a representing ideal of that class, accessed via self.ideal()
, and an expression for self in terms of the generators of its parent class group, accessed via self.list()
. The way you've set this up, elements of an S-class group use the same data structure, but the data which list()
uses is set completely wrongly: it's set to the exponents of the given ideal in terms of the generators of the class group, but it should be in terms of the generators of the S-class group. What you need to do is to store somewhere the expressions for the generators of the S-class group in terms of the generators of the class group (i.e. the matrix of the quotient map) and then multiply the output of _ideal_class_log
by this.
This is the sort of stuff that #6449 was intended to solve: quotients of abelian groups should be automatically handled by general code, rather than having to do it by hand in every specific example.
comment:10 Changed 13 years ago by
Replying to davidloeffler:
The AttributeError? comes up because attributes whose names start with an underscore are private, and ones with two underscores are "very private" -- so private that they aren't even accessible to subclasses. So when code in
FractionalIdealClass
sets__ideal
, code in the subclassSFractionalIdealClass
. In fact it is still accessible but under a mangled name: see http://docs.python.org/tutorial/classes.html#private-variables.But there is another, deeper, bug which will be uncovered when you fix this. After my changes at #9244, a
FractionalIdealClass
stores both a representing ideal of that class, accessed viaself.ideal()
, and an expression for self in terms of the generators of its parent class group, accessed viaself.list()
. The way you've set this up, elements of an S-class group use the same data structure, but the data whichlist()
uses is set completely wrongly: it's set to the exponents of the given ideal in terms of the generators of the class group, but it should be in terms of the generators of the S-class group. What you need to do is to store somewhere the expressions for the generators of the S-class group in terms of the generators of the class group (i.e. the matrix of the quotient map) and then multiply the output of_ideal_class_log
by this.This is the sort of stuff that #6449 was intended to solve: quotients of abelian groups should be automatically handled by general code, rather than having to do it by hand in every specific example.
David, you should have been at Sage Days!
comment:11 Changed 13 years ago by
Here's a patch that does the equivalent of _ideal_class_log
for S-class groups; you might find it useful.
comment:12 Changed 13 years ago by
Here's patch that finally makes the S-class-group work as a group, on top of Loeffler's patch(in this ticket). Soon to come will be a single patch that gets it all done, and actually has acceptable documentation.
comment:13 Changed 13 years ago by
Status: | needs_work → needs_review |
---|
comment:14 Changed 13 years ago by
Description: | modified (diff) |
---|
Correction: trac_9332_v3 should be applied only on trac_9244_ver4. Sorry for the confusion, trac_9332_v3 includes changes made in previous patches, so there's no need to apply them. Cheers.
comment:15 Changed 13 years ago by
Let's clean up this ticket. Shall I delete the patches other than trac_9332_v3
and trac_9244_ver4
?
comment:16 Changed 13 years ago by
All patches here can be deleted except trac_9332_v3
. (trac_9244_ver4
is from another ticket. )
comment:18 Changed 12 years ago by
Status: | needs_review → needs_work |
---|
Can we get this ticket moving again? The one and only patch still here does not apply cleanly to 4.6.rc0, so the first step is to fix that.
comment:19 Changed 12 years ago by
Authors: | Jim Stankewicz, Erin Beyerstedt, Anna Haensch → Jim Stankewicz, Erin Beyerstedt, Anna Haensch, Robert Miller, David Loeffler |
---|---|
Description: | modified (diff) |
I'm uploading a rebased (on sage-4.6) patch which modifies printing in class fields to not print the structure when size is 1, fixes some caching bugs, updates several doctests, etc...
However, this happens, and I'm not sure whether this is okay or not:
sage -t sage/rings/number_field/number_field_ideal.py ********************************************************************** File "/home/rlmill/sage-4.6/devel/sage-main/sage/rings/number_field/number_field _ideal.py", line 1017: sage: I._S_ideal_class_log([]) Expected: [3] Got: [1] **********************************************************************
comment:20 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
I applied the patch (no problems) to 4.6.1.alpha2. The thing you observed does not happen:
sage: K.<a> = QuadraticField(-14) sage: S = K.primes_above(2) sage: S [Fractional ideal (2, a)] sage: I = K.ideal(3, a-1) sage: I._S_ideal_class_log(S) [1] sage: I._S_ideal_class_log([]) [1]
I am now testing...
comment:21 Changed 12 years ago by
Reviewers: | → John Cremona |
---|
Two trivial doctest failures
sage -t -long sage/rings/polynomial/polynomial_quotient_ring.py ********************************************************************** File "/home/jec/sage-4.6.1.alpha2/devel/sage-main/sage/rings/polynomial/polynomial_quotient_ring.py", line 737: sage: K.class_group() Expected: Class group of order 1 with structure of Number Field in a with defining polynomial x^2 + 3 Got: Class group of order 1 of Number Field in a with defining polynomial x^2 + 3
and
sage -t -long sage/rings/number_field/number_field_ideal.py ********************************************************************** File "/home/jec/sage-4.6.1.alpha2/devel/sage-main/sage/rings/number_field/number_field_ideal.py", line 1051: sage: I._S_ideal_class_log([]) Expected: [3] Got: [1]
where in both cases the "Expected " does not look right anyway!
I am leaving this as needs review since I have not looked closely again at the code, but I can assert that apart from the above two things all (long) tests pass with 4.6.1.alpha2.
comment:22 Changed 12 years ago by
I also believe the actual output is correct. Evidence:
sage: K.<a> = QuadraticField(-14) sage: S = K.primes_above(2) sage: I = K.ideal(3,a+2) sage: I Fractional ideal (3, a + 2) sage: I._ideal_class_log() [1] sage: I._S_ideal_class_log([]) [1]
I am uploading a fresh patch, which will fix all the above issues.
comment:24 Changed 12 years ago by
Milestone: | → sage-4.6.2 |
---|
comment:25 Changed 12 years ago by
Status: | positive_review → needs_work |
---|
Nice patch but you should change the commit message :-)
comment:26 Changed 12 years ago by
Status: | needs_work → positive_review |
---|
comment:27 Changed 12 years ago by
Merged in: | → sage-4.6.2.alpha1 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:28 Changed 3 weeks ago by
Cc: | Alyson Deines added; adeines removed |
---|
I don't think there's a point to adding a TestSuite? doctest that doesn't work. We can always add that in once #7945 is fixed. You also need to make good on your IOU. I would also suggest a shorter
repr
string-- you don't want it line-wrapping on the terminal unnecessarily.