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

groups: improved elimination techniques for `reidemeister_ presentation` #12681

Merged
merged 16 commits into from Jun 20, 2017

Conversation

Projects
None yet
4 participants
@valglad
Contributor

valglad commented May 27, 2017

This PR rewrites the elimination_technique_1() and _simplification_technique_1() functions which leads to improved subgroup presentation for one of the tests. The application of these techniques in reidemester_presentation is changed so that they are applied until no further improvement is found instead of iterating 20 times (many cases would need less than that).

Also changes in the FreeGroupElement class have been made:

  • subword(from_i, to_j) now returns the subword beginning at position from_i and ending at position to_j as opposed to ending at position to_j - 1.
  • substituted_word() is modified in accordance with the new definition of subword (currently substituted_word is inconsistent about whether or not to_j is meant to be included in the subword or not).
  • eliminate_word() modified to handle substitutions for any subword, not just a generator
  • eliminate_words() is added to allow users to supply a dictionary of substitutions or a list of subwords that should be reduced to identity
  • subword_index() method is added. It returns the index of the first occurrence of a given subword in self
  • is_dependent() method is rewritten in terms of subword_index because the previous implementation depended on comparing string and that would fail for similarly named generators, for example b_1 and b_11 since str(b_1) in str(b_11) == True.
  • identity_cyclic_reduction() is modified to cyclically reduce the given word fully, not just once (e.g. previously (x**-1*y**-1*z*y*x).identity_cyclic_reduction() == y**-1*z*y instead of z)
@jksuom

This comment has been minimized.

Show comment
Hide comment
@jksuom

jksuom May 28, 2017

Member

I'll "wake up" Travis by closing and reopening.

Member

jksuom commented May 28, 2017

I'll "wake up" Travis by closing and reopening.

@jksuom jksuom closed this May 28, 2017

@jksuom jksuom reopened this May 28, 2017

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer May 29, 2017

Member

Why the subword change? The previous behavior matches standard Python indexing. It also is technically a backwards compatibility break.

Member

asmeurer commented May 29, 2017

Why the subword change? The previous behavior matches standard Python indexing. It also is technically a backwards compatibility break.

@valglad

This comment has been minimized.

Show comment
Hide comment
@valglad

valglad May 30, 2017

Contributor

@asmeurer, because the docstring said "the subword of self that begins at position from_i and ends at to_j" so I thought it was what was intended. Though considering that Python indexing is done the other way, it would probably be better to change the code back and just modify the docstring to be less misleading.

Contributor

valglad commented May 30, 2017

@asmeurer, because the docstring said "the subword of self that begins at position from_i and ends at to_j" so I thought it was what was intended. Though considering that Python indexing is done the other way, it would probably be better to change the code back and just modify the docstring to be less misleading.

@jksuom

This comment has been minimized.

Show comment
Hide comment
@jksuom

jksuom Jun 14, 2017

Member

I think this could be merged once the conflicts are resolved.

Member

jksuom commented Jun 14, 2017

I think this could be merged once the conflicts are resolved.

@jksuom jksuom merged commit 985496e into sympy:master Jun 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@valglad valglad deleted the valglad:eliminations branch Jun 20, 2017

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