Opened 8 years ago

Closed 8 years ago

#14791 closed enhancement (fixed)

Named finitely presented groups

Reported by: dshurbert Owned by: joyner
Priority: minor Milestone: sage-5.13
Component: group theory Keywords: group presentations, free groups
Cc: rbeezer, vbraun, mmarco Merged in: sage-5.13.beta0
Authors: Davis Shurbert Reviewers: Volker Braun, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by tscrim)

Using the existing groups catalog framework, users are given the capability to create "named" groups as finite presentations. The goal of this addition to Sage is to make the concepts of free groups and group presentations more accessible to the typical undergrad, allowing them to work with standard groups introduced in an introductory level abstract algebra course.

sage: groups.presentation.Cyclic(17)
Finitely presented group < a | a^17 >
sage: groups.presentation.Dihedral(8)
Finitely presented group < a, b | a^8, b^2, a*b*a*b >
sage: groups.presentation.DiCyclic(5)
Finitely presented group < a, b | a^10, b^2*a^-5, b^-1*a*b*a >
sage: groups.presentation.KleinFour()
Finitely presented group < a, b | a^2, b^2, a^-1*b^-1*a*b >

More groups are to be implemented soon. Feedback/suggestions on implementation details are highly encouraged.

Apply

Attachments (3)

trac_14791_named_fpgroups.patch (7.6 KB) - added by dshurbert 8 years ago.
Patch
trac_14791_named_fpgroups.2.patch (9.0 KB) - added by dshurbert 8 years ago.
Replacement patch
trac_14791-review-ts.patch (14.8 KB) - added by tscrim 8 years ago.

Download all attachments as: .zip

Change History (26)

Changed 8 years ago by dshurbert

Patch

comment:1 Changed 8 years ago by dshurbert

  • Description modified (diff)

comment:2 Changed 8 years ago by dshurbert

  • Status changed from new to needs_info

comment:3 Changed 8 years ago by dshurbert

  • Description modified (diff)

comment:4 Changed 8 years ago by dshurbert

  • Status changed from needs_info to needs_review

comment:5 Changed 8 years ago by vbraun

The printing of free group elements changed in the latest GAP update, this leads to doctest failures of the kind

Failed example:
    D = groups.presentation.Dihedral(7); D
Expected:
    Finitely presented group < a, b | a^7, b^2, a*b*a*b >
Got:
    Finitely presented group < a, b | a^7, b^2, (a*b)^2 >

The patch should be rebased to the latest sage beta (5.11.beta3 as of today)

Minor nitpick: Don't end docstrings with a blank line. It just wastes vertical space needlessly. The """ end marker is enough visual cue that the docstring is finished.

comment:6 Changed 8 years ago by dshurbert

Fixed doctest failures caused by the new printing of free group elements.

Version 0, edited 8 years ago by dshurbert (next)

comment:7 Changed 8 years ago by vbraun

I get a big fat warning that libgap is being loaded on Sage startup. You should lazy_import in sage/groups/groups_catalog.py.

comment:8 Changed 8 years ago by dshurbert

The following code placed in sage/groups/groups_catalog.py:

from sage.misc.lazy_import import lazy_import
lazy_import( 'sage.groups', 'finitely_presented_catalog', 'presentation' )

Has the desired effect (libgap is no longer loaded on startup), however groups.lazy_import is now part of the global name-space. Any suggestions for removing this side-effect?

comment:9 Changed 8 years ago by jhpalmieri

How about this change instead:

  • sage/groups/all.py

    diff --git a/sage/groups/all.py b/sage/groups/all.py
    a b  
    2121lazy_import('sage.groups.affine_gps.affine_group', 'AffineGroup')
    2222lazy_import('sage.groups.affine_gps.euclidean_group', 'EuclideanGroup')
    2323
    24 import groups_catalog as groups
     24lazy_import('sage.groups', 'groups_catalog', 'groups')

comment:10 Changed 8 years ago by vbraun

Yes, the whole groups catalog should be lazy.

comment:11 Changed 8 years ago by dshurbert

Just made the groups_catalog import in sage/groups/all.py lazy. Added some documentation and more tests.

comment:12 Changed 8 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

Looks good to me

comment:13 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

The patch needs a proper commit message.

comment:14 Changed 8 years ago by jdemeyer

There is a conflict with #14910: both patches add the same reference [TWGT].

Changed 8 years ago by dshurbert

Replacement patch

comment:15 Changed 8 years ago by dshurbert

  • Status changed from needs_work to needs_review

Added commit message, fixed reference conflict.

Changed 8 years ago by tscrim

comment:16 Changed 8 years ago by tscrim

  • Reviewers changed from Volker Braun to Volker Braun, Travis Scrimshaw

Hey,

Here's a small review patch which fixes a few typos and formatting in the documentation, and if you're happy with my changes, we can set this (back to) positive review.

Best,
Travis

comment:17 Changed 8 years ago by dshurbert

Looks good to me, thank you for the catches.

comment:18 Changed 8 years ago by tscrim

  • Status changed from needs_review to positive_review

I'm going to interpret that as a positive review.

comment:19 Changed 8 years ago by tscrim

  • Description modified (diff)

comment:20 Changed 8 years ago by tscrim

  • Description modified (diff)

comment:21 Changed 8 years ago by tscrim

For patchbot:

Apply: trac_14791_named_fpgroups.2.patch, trac_14791-review-ts.patch

comment:22 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.12 to sage-5.13

comment:23 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.13.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.