Opened 12 years ago
Closed 11 years ago
#4739 closed enhancement (duplicate)
[fixed by #6449] Add support for additive abelian groups
Reported by: | cremona | Owned by: | tbd |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | algebra | Keywords: | abelian group |
Cc: | robertwb | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
The patch adds code to the AbelianGroups_class so that additive groups can be created, and are output with additive notation.
Further doctests are required to show examples of both additive and multiplicative type in every case. Subgroup creation (which interfaces with GAP) does not yet work.
As an example (more to come), torsion subgroups of elliptic curves over number fields are now additive.
robertwb asked to have a look at this even though it is not yet finished, so here it is!
NB I still think that the whole abelian group code needs rewriting (as was started in #1849)!
Attachments (1)
Change History (9)
Changed 12 years ago by
comment:1 follow-up: ↓ 2 Changed 12 years ago by
Hi John,
I haven't look at the code too seriously, but what would be your thoughts on having two subclasses (additive and multiplicative) of a common class for the abelian group elements so that one doesn't need to always do a check for the parent's op_symbol?
--Mike
comment:2 in reply to: ↑ 1 Changed 12 years ago by
Replying to mhansen:
Hi John,
I haven't look at the code too seriously, but what would be your thoughts on having two subclasses (additive and multiplicative) of a common class for the abelian group elements so that one doesn't need to always do a check for the parent's op_symbol?
--Mike
That would be a very good idea indeed. The way I see it, there is a base abstract class (in which elements are represented as integer vectors, operation is addition with a modulus for each coordinate corresponding to a generator of finite order); and then there are two possible interfaces to that base class via user classes which are either additive or multiplicative.
We are surely in a position to provide this structure right now, using the exiting implementations for the base class. Then the "major rewrite" which we keep on mentioning need only apply to the base class.
I'll continue to think about this; it does not sound any harder, and is probably easier, than what I have been doing so far.
comment:3 Changed 12 years ago by
I agree with mhansen, always checking parent's op_symbol is probably slow, but I think this can be avoided.
Note that we already have two classes, AdditiveGroupElement? and MultiplicativeGroupElement?. I think the code here is trying to unify them. both under MultiplicativeGroupElement?. Also, note that no typechecking (or coercion) is done for the add and sub commands, so if the left and right operand are not the correct type (or, if both are abelian group elements, but in different groups) then either an error or nonsense will be returned.
For multiplication, do we really want a * (1+5+O(5^2))
and a * "50"
to work for a group element a?
comment:4 Changed 11 years ago by
- Summary changed from [with patch, not ready for review] Add support for additive abelian groups to Add support for additive abelian groups
comment:5 follow-up: ↓ 6 Changed 11 years ago by
- Report Upstream set to N/A
- Status changed from needs_work to needs_review
- Summary changed from Add support for additive abelian groups to [fixed by #6449] Add support for additive abelian groups
Can I propose we resolve this as "fixed" now that #6449 has gone in?
comment:6 in reply to: ↑ 5 Changed 11 years ago by
Replying to davidloeffler:
Can I propose we resolve this as "fixed" now that #6449 has gone in?
That sounds reasonable to me. There's no chance that this ancient patch would apply now anyway.
comment:7 Changed 11 years ago by
- Status changed from needs_review to positive_review
OK, I'm setting it to "positive review" to bring it to the attention of the release manager who can close it.
comment:8 Changed 11 years ago by
- Milestone changed from sage-4.5.2 to sage-duplicate/invalid/wontfix
- Resolution set to duplicate
- Status changed from positive_review to closed
If no one objects, I'll close this ticket as a "duplicate."
Applies to 3.2.1 after #3810 and #4061 patches.