Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FreeGroup(0)([]) broken #17246

Closed
darijgr opened this issue Oct 28, 2014 · 15 comments
Closed

FreeGroup(0)([]) broken #17246

darijgr opened this issue Oct 28, 2014 · 15 comments

Comments

@darijgr
Copy link
Contributor

darijgr commented Oct 28, 2014

This is a border case, but I'd still prefer it to work:

sage: F = FreeGroup(0)
sage: F([])
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-47-49d58f580ab1> in <module>()
----> 1 F([])

/scratch/sage-6.4.beta2/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9603)()

/scratch/sage-6.4.beta2/local/lib/python2.7/site-packages/sage/structure/coerce_maps.so in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4256)()

/scratch/sage-6.4.beta2/local/lib/python2.7/site-packages/sage/structure/coerce_maps.so in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4163)()

/scratch/sage-6.4.beta2/local/lib/python2.7/site-packages/sage/groups/free_group.pyc in _element_constructor_(self, *args, **kwds)
    808             P = x.parent()
    809         except AttributeError:
--> 810             return self.element_class(self, x, **kwds)
    811         if isinstance(P, FreeGroup_class):
    812             names = set(P._names[abs(i)-1] for i in x.Tietze())

/scratch/sage-6.4.beta2/local/lib/python2.7/site-packages/sage/groups/free_group.pyc in __init__(self, parent, x)
    225                     i=i+1
    226             AbstractWordTietzeWord = libgap.eval('AbstractWordTietzeWord')
--> 227             x = AbstractWordTietzeWord(l, parent._gap_gens())
    228         ElementLibGAP.__init__(self, parent, x)
    229 

/scratch/sage-6.4.beta2/local/lib/python2.7/site-packages/sage/libs/gap/element.so in sage.libs.gap.element.GapElement_Function.__call__ (build/cythonized/sage/libs/gap/element.c:15598)()

ValueError: libGAP: Error, List Element: <list>[1] must have an assigned value

CC: @tscrim

Component: group theory

Keywords: gap, group, free group, border case

Author: Travis Scrimshaw

Branch/Commit: 40c165c

Reviewer: Darij Grinberg

Issue created by migration from https://trac.sagemath.org/ticket/17246

@darijgr darijgr added this to the sage-6.4 milestone Oct 28, 2014
@tscrim
Copy link
Collaborator

tscrim commented Oct 29, 2014

comment:1

I will get a fix in a day or two.

@tscrim
Copy link
Collaborator

tscrim commented Nov 4, 2014

comment:2

I made the input to element construct just return the identity anytime a bool(x) is False (which includes empty lists and tuples), so it fixes the issue (and possibly slightly faster than before).


New commits:

9121d13Fix for trivial free group.

@tscrim
Copy link
Collaborator

tscrim commented Nov 4, 2014

Commit: 9121d13

@tscrim
Copy link
Collaborator

tscrim commented Nov 4, 2014

Author: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Nov 4, 2014

@darijgr
Copy link
Contributor Author

darijgr commented Nov 7, 2014

comment:3

Looks good to me, though a shade of doubt remains about whether not x doesn't encompass a few cases too many. Can you run this by someone who knows Sage's free groups well?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2014

Changed commit from 9121d13 to c65cb5e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2014

Branch pushed to git repo; I updated commit sha1. New commits:

c65cb5ePrevent 0 from being used to construct the identity.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2014

Branch pushed to git repo; I updated commit sha1. New commits:

40c165cChanged test to explicitly check x == [] or x == ().

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2014

Changed commit from c65cb5e to 40c165c

@tscrim
Copy link
Collaborator

tscrim commented Nov 7, 2014

comment:6

It did overextend to include 0 going to the identity. However the input is suppose to be a list or tuple whose entries correspond to (inverse) generators, so I've changed it to explicitly check x == [] or x == ().

@darijgr
Copy link
Contributor Author

darijgr commented Nov 7, 2014

comment:7

Thanks!

@tscrim
Copy link
Collaborator

tscrim commented Nov 7, 2014

comment:8

NP. Thanks for doing the review.

@tscrim
Copy link
Collaborator

tscrim commented Nov 7, 2014

Reviewer: Darij Grinberg

@vbraun
Copy link
Member

vbraun commented Nov 15, 2014

Changed branch from public/groups/trivial_free_group-17246 to 40c165c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants