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

Fix bug in WordMorphism.growing_letters() and periodic_points() #31454

Closed
mrejmon mannequin opened this issue Mar 5, 2021 · 8 comments
Closed

Fix bug in WordMorphism.growing_letters() and periodic_points() #31454

mrejmon mannequin opened this issue Mar 5, 2021 · 8 comments

Comments

@mrejmon
Copy link
Mannequin

mrejmon mannequin commented Mar 5, 2021

sage: WordMorphism('a->a').is_growing('a')
True

should return False.

This happens due to the if self.is_primitive(): return True optimization. Easily fixable for example like this:

-       if self.is_primitive():
+       if self.is_primitive() and len(self._morph) > 1:
            return True
sage: WordMorphism('a->a,b->bb').periodic_points()
Traceback (most recent call last):
...
ValueError: generator already executing

should return [[word: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb...]].

I think this happens because periodic_points() doesn't check whether the letters are growing, so the following should fix it (assuming we only care about periodic points with infinite length):

        A = self.domain().alphabet()
        d = dict((letter,self(letter)[0]) for letter in A)
+       G = set(self.growing_letters())

        res = []
        parent = self.codomain().shift()
        for cycle in get_cycles(CallableDict(d),A):
+           if cycle[0] in G:
                P = PeriodicPointIterator(self, cycle)
                res.append([parent(P._cache[i]) for i in range(len(cycle))])

I added a branch with the proposed fixes.

CC: @seblabbe @slel

Component: combinatorics

Author: Martin Rejmon

Branch/Commit: 87a8aae

Reviewer: Vincent Delecroix

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

@mrejmon mrejmon mannequin added this to the sage-9.3 milestone Mar 5, 2021
@mrejmon mrejmon mannequin added c: combinatorics labels Mar 5, 2021
@seblabbe
Copy link
Contributor

seblabbe commented Mar 8, 2021

comment:3

Thanks for proposing such fixes. I will take a look as soon as possible.

@videlec
Copy link
Contributor

videlec commented Mar 14, 2021

comment:4

Martin. Your branch should include the two doctests that are in the ticket description. Since the sage doctests are run it will prevent the bug to occur again in the future.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 16, 2021

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

87a8aaeAdd regression tests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 16, 2021

Changed commit from 085165a to 87a8aae

@mrejmon
Copy link
Mannequin Author

mrejmon mannequin commented Mar 16, 2021

comment:6

Replying to @videlec:

Martin. Your branch should include the two doctests that are in the ticket description. Since the sage doctests are run it will prevent the bug to occur again in the future.

Done.

@videlec
Copy link
Contributor

videlec commented Mar 16, 2021

Reviewer: Vincent Delecroix

@videlec
Copy link
Contributor

videlec commented Mar 16, 2021

comment:7

thanks!

@vbraun
Copy link
Member

vbraun commented Mar 18, 2021

Changed branch from u/gh-mrejmon/fix_growing_and_periodic to 87a8aae

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