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

group theory: PermutationGroup methods for FpGroup #13119

Merged
merged 4 commits into from Aug 22, 2017

Conversation

Projects
None yet
2 participants
@valglad
Contributor

valglad commented Aug 13, 2017

This PR implements some of the methods from the PermutationGroup class for the FpGroup class. This is done by using coset tables to find an isomorphism from an FpGroup to a PermutationGroup, applying the method in the latter and then using the isomorphism to go back. Additionally, homomorphisms can now invert or be applied to a list of elements.

@jksuom

This comment has been minimized.

Show comment
Hide comment
@jksuom

jksuom Aug 21, 2017

Member

It would be possible to make _perm_group_list work also with methods that return a single group.

from sympy.core.compatibility import is_sequence
...

single = False
if not is_sequence(perm_result):
    perm_result, single = [perm_result], True

...
return result[0] if single else result

Then it should not be necessary to define method arguments as lambda expressions, the name (and getattr) would suffice.

Member

jksuom commented Aug 21, 2017

It would be possible to make _perm_group_list work also with methods that return a single group.

from sympy.core.compatibility import is_sequence
...

single = False
if not is_sequence(perm_result):
    perm_result, single = [perm_result], True

...
return result[0] if single else result

Then it should not be necessary to define method arguments as lambda expressions, the name (and getattr) would suffice.

@valglad

This comment has been minimized.

Show comment
Hide comment
@valglad

valglad Aug 22, 2017

Contributor

It seems that a single PemutationGroup is a sequence. Iterating over it is the same as iterating over its generators. It's a strange set-up. I could remove its __getitem__ method but it probably has to be deprecated first. Alternatively, I can do something like hasattr(perm_result, 'generators') to check if single is True.

Contributor

valglad commented Aug 22, 2017

It seems that a single PemutationGroup is a sequence. Iterating over it is the same as iterating over its generators. It's a strange set-up. I could remove its __getitem__ method but it probably has to be deprecated first. Alternatively, I can do something like hasattr(perm_result, 'generators') to check if single is True.

@jksuom

This comment has been minimized.

Show comment
Hide comment
@jksuom

jksuom Aug 22, 2017

Member

Would it be possible to use isinstance(perm_result, PermutationGroup)?

Member

jksuom commented Aug 22, 2017

Would it be possible to use isinstance(perm_result, PermutationGroup)?

@valglad

This comment has been minimized.

Show comment
Hide comment
@valglad

valglad Aug 22, 2017

Contributor

Yes, that would be clearer. Is there any other reason to use isinstance as opposed to hasattr in general?

Contributor

valglad commented Aug 22, 2017

Yes, that would be clearer. Is there any other reason to use isinstance as opposed to hasattr in general?

@valglad

This comment has been minimized.

Show comment
Hide comment
@valglad

valglad Aug 22, 2017

Contributor

That would require importing PermutationGroup inside the function, but I suppose that's not a problem?

Contributor

valglad commented Aug 22, 2017

That would require importing PermutationGroup inside the function, but I suppose that's not a problem?

@jksuom

This comment has been minimized.

Show comment
Hide comment
@jksuom

jksuom Aug 22, 2017

Member

Why should if be imported inside the function?

Member

jksuom commented Aug 22, 2017

Why should if be imported inside the function?

@jksuom

This comment has been minimized.

Show comment
Hide comment
@jksuom

jksuom Aug 22, 2017

Member

Is there any other reason to use isinstance as opposed to hasattr in general?

I think it is generally safer to use isinstance.

Member

jksuom commented Aug 22, 2017

Is there any other reason to use isinstance as opposed to hasattr in general?

I think it is generally safer to use isinstance.

@valglad

This comment has been minimized.

Show comment
Hide comment
@valglad

valglad Aug 22, 2017

Contributor

Why should if be imported inside the function?

Because it's not imported at the beginning of the file. It has to be imported somewhere, and it's only used in this function.

Contributor

valglad commented Aug 22, 2017

Why should if be imported inside the function?

Because it's not imported at the beginning of the file. It has to be imported somewhere, and it's only used in this function.

@jksuom

This comment has been minimized.

Show comment
Hide comment
@jksuom

jksuom Aug 22, 2017

Member

Imports should be at the top of the file also when they are used in only one function. Sometimes that may lead to circular imports, and then they have to be placed inside functions.

Member

jksuom commented Aug 22, 2017

Imports should be at the top of the file also when they are used in only one function. Sometimes that may lead to circular imports, and then they have to be placed inside functions.

@valglad

This comment has been minimized.

Show comment
Hide comment
@valglad

valglad Aug 22, 2017

Contributor

I see, will change it then.

Contributor

valglad commented Aug 22, 2017

I see, will change it then.

@jksuom jksuom merged commit 662cfb8 into sympy:master Aug 22, 2017

1 check passed

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

@valglad valglad deleted the valglad:perm_to_fp branch Aug 22, 2017

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