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:

Status badges

Description

See also #1849 and #3999.

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)

abgr.patch (45.7 KB) - added by cremona 12 years ago.
Applies to 3.2.1 after #3810 and #4061 patches.

Download all attachments as: .zip

Change History (9)

Changed 12 years ago by cremona

Applies to 3.2.1 after #3810 and #4061 patches.

comment:1 follow-up: Changed 12 years ago by 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

comment:2 in reply to: ↑ 1 Changed 12 years ago by cremona

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 robertwb

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 AlexGhitza

  • 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: Changed 11 years ago by davidloeffler

  • 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 cremona

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 davidloeffler

  • 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 mpatel

  • 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."

Note: See TracTickets for help on using tickets.