Opened 10 years ago

Closed 10 years ago

#14081 closed defect (fixed)

Solve bug in BraidGroup

Reported by: Miguel Marco Owned by: joyner
Priority: major Milestone: sage-5.9
Component: group theory Keywords:
Cc: Jeroen Demeyer, Volker Braun Merged in: sage-5.9.beta4
Authors: Miguel Marco Reviewers: Travis Scrimshaw, Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

BraidGroup? checks that the number of generators is bigger or equal than 2. It should be 1 (two strands, one generator). So, now the braid group on two strands can't be created.

This patch solves it. (It is just a one line change):

Apply: 14081_one_generator.patch​ and 14081_one_generator2.patch​

Attachments (2)

14081_one_generator.patch​ (977 bytes) - added by Miguel Marco 10 years ago.
14081_one_generator2.patch​ (905 bytes) - added by Miguel Marco 10 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by Travis Scrimshaw

Authors: → Miguel Marco
Reviewers: → Travis Scrimshaw

Could you add a doctest showing this is fixed and a comment on that about about why it should be n < 1 instead of n < 2 as the error message (correctly) indicates (such as # n now is the number of generators)? Once that is done, set this to needs_review.

Thanks,
Travis

Changed 10 years ago by Miguel Marco

Attachment: 14081_one_generator.patch​ added

comment:2 Changed 10 years ago by Miguel Marco

Status: new → needs_review

Apply 14981_one_generator.patch

comment:3 Changed 10 years ago by Volker Braun

Description: modified (diff)

comment:4 Changed 10 years ago by Volker Braun

For the patchbot:

Apply 14081_one_generator.patch

comment:5 Changed 10 years ago by Travis Scrimshaw

Description: modified (diff)

There's also a doctest failure (due to capitalization). Also, for the doctest, could you make it some form of: 

Check that :trac:`14081` is fixed::

    sage: BraidGroup(2)
    Braid group on 2 strands
    sage: BraidGroup(('a',))
    Braid group on 2 strands

Thanks.

Changed 10 years ago by Miguel Marco

Attachment: 14081_one_generator2.patch​ added

comment:6 Changed 10 years ago by Miguel Marco

Description: modified (diff)

comment:7 Changed 10 years ago by Volker Braun

Reviewers: Travis Scrimshaw → Travis Scrimshaw, Volker Braun
Status: needs_review → positive_review

Looks good to me

comment:8 Changed 10 years ago by Travis Scrimshaw

Same. Thank you for your work Miguel and the final review Volker.

comment:9 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

comment:10 Changed 10 years ago by Jeroen Demeyer

Merged in: → sage-5.9.beta4
Resolution: → fixed
Status: positive_review → closed
Note: See TracTickets for help on using tickets.