Opened 10 years ago
Closed 10 years ago
#13687 closed enhancement (fixed)
Parents for groups
Reported by: | Volker Braun | Owned by: | joyner |
---|---|---|---|
Priority: | major | Milestone: | sage-5.7 |
Component: | group theory | Keywords: | new Parent |
Cc: | Miguel Marco, David Roe, David Joyner, Simon King | Merged in: | sage-5.7.beta2 |
Authors: | Volker Braun | Reviewers: | David Roe |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Groups are based on the deprecated ParentWithGens
. This ticket is about forward-porting them to the new coercion framework.
To limit the size of the patchbomb I'm only porting the Abelian groups. The rest depends on old.Group
Various oddities needed to be cleaned up in the process:
- make parents unique and elements immutable (often requires tuples instead of lists)
- rename
AbelianGroup_class.invariants()
->AbelianGroup_class.gens_orders()
since they are not necessarily the abelian invariants (=elementary divisors). - rename
element.list()
->element.exponents()
for the exponent vector. - implement a
AbelianGroupWithValues
where you attach values to the (abstract) generators. Used in the unit group of a number field, for example. - lots of places return lists when they should return tuples
I haven't actually deprecated .invariants()
and .list()
, just changed the documentation to suggest the equivalent .gens_orders()
and .exponents()` methods. But I think we should deprecate them. Though I first want to work my way through the remaining groups code.
Apply
Attachments (6)
Change History (41)
comment:1 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 10 years ago by
Status: | new → needs_review |
---|
comment:3 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:4 Changed 10 years ago by
Authors: | → Volker Braun |
---|---|
Cc: | Miguel Marco David Roe David Joyner Simon King added |
Keywords: | new Parent added |
comment:5 Changed 10 years ago by
Reviewers: | → David Roe |
---|---|
Status: | needs_review → needs_work |
comment:6 Changed 10 years ago by
- The 0's were prepended, I just kept that behavior. I think its more in line with the mathematical expectation of writing a group as free+torsion instead of the other way round. Also, the
gens_orders
are not the abelian invariants.
- I still haven't figured out what
invs()
is for, I'll just delete it ;-)
I agree with the rest. Most of it is just stuff that was already there and I didn't fix...
comment:7 Changed 10 years ago by
- Fair enough.
- Sounds good.
I know, I saw the old code. But if you're making changes this extensive you might as well fix some more stuff. :-)
comment:8 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → needs_review |
comment:9 Changed 10 years ago by
Here are some more suggestions. I'm going to leave it marked as needs review so that the patchbot will continue testing.
- In
AbelianGroupElement.__init__
, I think it's useful to allowints
in thegenerator_orders
, rather than asserting that they all beIntegers
- Documentation of
exponents
doesn't line up with__init__
method inAbelianGroupElementBase
. You should give an example in the documentation of that function showing that the exponents are still integers, not elements of the base ring.
- In docstring for
AbelianGroupWithValues
, maybe you should note thatvalues_group
could be a ring or field, since Sage doesn't currently support the group of units in a ring very well.
- I don't think that
AbelianGroupWithValuesEmbedding
should inherit fromCallMorphism
, but rather just fromMorphism
.
- For the arithmetic operations in
AbelianGroupWithValuesElement
, I would keep the computation of_value
lazy (check to see if both left and right values are not None). By being nonlazy you'll get more floating point drift and also you may compute unnecessary values.
- Your docstring for
AbelianGroupWithValues_class.values_group
is incomplete.
- Indentation problem at the end of the docstring for
Group
(right beforedef __init__
)
- You set
Element = FractionalIdealClass
onSFractionalIdealClass(FractionalIdealClass)
. This should be done on the parent, not the element.
- There doesn't seem to be a failure in the test suite for
ClassGroup
, so you should remove the sentence about a failure caused by not having a category of additive abelian groups.
- In
unit_group.py
you use_.value()
a few times; can you giveUK.torsion_generator()
(etc) a name and use it instead of_
?
- You have a docstring floating that needs to be deleted where
gen(self, i=0)
used to be inUnitGroup
comment:10 Changed 10 years ago by
AbelianGroup_class
derives fromUniqueRepresentation
, so I want to enforce canonical arguments with the assertions. Its the job of theAbelianGroup
factory function to canonicalize the user input.
- For the unit group, say, its much more expensive to do the exponentiation than non-lazy multiplication. Imagine you want to list all units. This is also what the
FractionalIdealClass
and friends do. If its cheap to exponentiate the values then I would agree with you, but in that case you probably don't need such an elaborateAbelianGroupWithValues
.
comment:11 Changed 10 years ago by
The updated patch addresses all remaining issues. I've also based ClassGroup
on AbelianGroupWithValues
and some other improvements.
comment:12 Changed 10 years ago by
Looks good. Two more comments and then I'm happy to give a positive review:
- In docstring for
SFractionalIdealClass
, your example blocks are indented. There should also only be one block rather than three.
- There's a failure in the startup plugin. I have no problem giving a positive review even so, but I thought I'd point it out in case you wanted to change anything related to imports. :-)
comment:13 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
Good eyes with the indent ;-)
I noticed that startup modules changed. Fun fact: the operator
Python module (for operator.mul
, say) was imported as a by-product of Abelian groups. And the coercion framework assumes that it is imported in its doctests. I've moved the actual import to sage.all
, as you saw. In any case, I'm planning to make everything lazy if possible, but first I should go through the rest of the groups and port them to the new parents.
comment:14 Changed 10 years ago by
Sounds good. Let me know when you need a review for the transition on the other groups. Thanks for all the changes here: fixing Groups has been a daunting task.
comment:16 Changed 10 years ago by
Status: | positive_review → needs_work |
---|
Long doctests fail:
sage -t -long "devel/sage/sage/rings/number_field/number_field_ideal.py" *** Warning: precision too low for generators, not given. ********************************************************************** File "/release/merger/sage-5.5.beta1/devel/sage/sage/rings/number_field/number_field_ideal.py", line 2462: sage: G.gens_values() Expected: (2*a^2 - 1, 2*a^2 + 2*a - 2) Got: (2*a - 1, 2*a^2 + 2*a - 2) **********************************************************************
comment:17 Changed 10 years ago by
Status: | needs_work → positive_review |
---|
I've marked the offending doctest as # random
, which is what it was before the patch.
comment:18 Changed 10 years ago by
This patch introduces some bad uses of assert
. You should only use "assert" to check for actual bugs, not for bad user input. If there is any way at all to reproduce an AssertionError using documented public functions, that is by definition a bug.
comment:19 Changed 10 years ago by
Status: | positive_review → needs_work |
---|---|
Work issues: | → bad use of assert |
comment:20 Changed 10 years ago by
Most importantly, you must never use assert
for control flow. So the following is very wrong:
try: ... except AssertionError: ...
comment:21 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → needs_review |
Work issues: | bad use of assert |
Agreed, but its only "introduced" in the sense that I copied the old groups.pyx
to old.pyx
to support old-style parents until every group is switched over. I've cleaned up the AssertionError
in the new group.pyx
but not in the old.pyx
.
comment:22 Changed 10 years ago by
comment:23 Changed 10 years ago by
I have based the patches on sage-5.5.rc0. Can you give that a try?
comment:25 Changed 10 years ago by
So, Jeroen, would you be happy with a positive review now that the assertion issue is cleaned from the new code, but kept in the old one? Or should we make the full transition of groups before accepting this?
comment:26 Changed 10 years ago by
Description: | modified (diff) |
---|
I combined and rebased the patches and marked old.pyx
and old.pxd
as copies.
Changed 10 years ago by
Attachment: | 13687_parent_for_groups_all.patch added |
---|
comment:27 Changed 10 years ago by
It's not required to fix all of old.pyx
. If the newly introduced assertions are fine, then that's sufficient.
comment:28 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
I'm fine with the rebasing... don't think there is anything else to review.
comment:29 Changed 10 years ago by
Status: | positive_review → needs_work |
---|
This needs to rebased to sage-5.7.beta1.
comment:31 Changed 10 years ago by
Status: | needs_work → positive_review |
---|
I ran into a strange issue here: http://tracker.gap-system.org/issues/224. I've added a larger initial workspace size (-m 64m
) which should be a workaround. In any case it has nothing to do with upgrading the group parents.
comment:32 Changed 10 years ago by
Status: | positive_review → needs_work |
---|
There are 2 failures on 32-bit:
sage -t --long -force_lib devel/sage/sage/rings/number_field/unit_group.py ********************************************************************** File "/var/lib/buildbot/build/sage/arando-1/arando_full/build/sage-5.7.beta2/devel/sage-main/sage/rings/number_field/unit_group.py", line 138: sage: u.value() Expected: -1/4*a^3 - 7/4*a^2 - 17/4*a - 19/4 Got: -1/4*a^3 + 7/4*a^2 - 17/4*a + 19/4 ********************************************************************** sage -t --long -force_lib devel/sage/sage/rings/polynomial/polynomial_quotient_ring.py ********************************************************************** File "/var/lib/buildbot/build/sage/arando-1/arando_full/build/sage-5.7.beta2/devel/sage-main/sage/rings/polynomial/polynomial_quotient_ring.py", line 974: sage: S.S_class_group([K.ideal(a)]) Expected: [((2, -a + 1, 1/2*xbar + 1/2, -1/2*a*xbar + 1/2*a + 1), 6, 1/2*xbar - 3/2)] Got: [((2, -a + 1, 1/2*xbar + 1/2, -1/2*a*xbar + 1/2*a + 1), 6, -1/2*xbar + 3/2)] **********************************************************************
comment:33 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → positive_review |
I've added the necessary # 32-bit
clauses...
comment:34 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:35 Changed 10 years ago by
Merged in: | → sage-5.7.beta2 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Hi Volker, I'm about halfway through this. Overall it's looking great. Here are some comments (let me know if anything is unclear):
_normalize
function forAbelianGroup
, perhaps the 0s should come last, since that lines up with the standard divisibility condition for invariants. Also, ifgens_orders
is a string of lengthn
then it will get through your error checking._normalize
should have examples.random_element
, shouldn't the fact that doctesting consistently chooses a random seed mean we don't need to markG.random_element()
as# random output
?equals
, the documentation for the output is wrong: should be checking whether they are the same subset.invs()
, where you writeraise Exception # WTF
, maybe we should figure out what was intended by this function and fix it. :-)dual_abelian_group
, you can use floating point tolerance rather than just marking the output as random. In particular, shouldn't your first example be exact?dual_abelian_group_element
, you should take out the# random
marking on some doctests (using floating point tolerance instead if necessary)I should go to sleep, but I'll try to get through more of this tomorrow. When you make changes, could you upload a separate patch?