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 One/Zero/MultiplicativeZero for partial perms #1040

Merged
merged 1 commit into from
Jan 3, 2017

Conversation

james-d-mitchell
Copy link
Contributor

@james-d-mitchell james-d-mitchell commented Dec 23, 2016

Please make sure that this pull request:

  • is submitted to the correct branch (the stable branch is only for bugfixes)
  • contains an accurate description of changes for the release notes below
  • provides new tests or relies on existing ones
  • correctly refers to other issues and related pull requests

Tick all what applies to this pull request

  • Adds new features
  • Improves and extends functionality
  • Fixes bugs that could lead to crashes
  • Fixes bugs that could lead to incorrect results
  • Fixes bugs that could lead to break loops

Write below the description of changes (for the release notes)

There were several issues here:

  1. some declarations were missing

  2. for a partial perm semigroup without generators the default method or
    a One of a partial perm collection was used. This always returned an
    answer, when it shouldn't have it if the One was not in the semigroup.

  3. partial perms and semigroups of partial perms had methods for Zero,
    which according to the manual "return the additive neutral element of
    the additive element". Since partial perms are not additive these
    methods were inappropriate. As such they are removed in this PR and
    methods for MultiplicativeZero are introduced instead. Partial perms
    should have belonged to the category IsMultiplicativeElementWithZero,
    and so I added this to the definition of IsPartialPerm in pperm.g.
    This meant moving the declaration of IsMultiplicativeElementWithZero
    from mgmadj.gd to arith.gd so that it is available when pperm.g is
    read (from read1.g). There was no default method for the attribute
    MultiplicativeZero for an object in IsMultipicativeElementWithZero
    so I added one of these too, it simply calls MultiplicativeZeroOp.

This commit fixes these issues.

@james-d-mitchell james-d-mitchell changed the title Fix One/Zero/MultiplicativeZero for partial perms WIP Fix One/Zero/MultiplicativeZero for partial perms Dec 23, 2016
@codecov-io
Copy link

codecov-io commented Dec 23, 2016

Current coverage is 49.60% (diff: 100%)

Merging #1040 into master will decrease coverage by <.01%

@@             master      #1040   diff @@
==========================================
  Files           424        424          
  Lines        223277     223266    -11   
  Methods        3430       3429     -1   
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits         110772     110760    -12   
- Misses       112505     112506     +1   
  Partials          0          0          

Powered by Codecov. Last update 34ef6a1...f148dac

There were several issues here:

1. some declarations were missing

2. for a partial perm semigroup without generators the default method or
a One of a partial perm collection was used. This always returned an
answer, when it shouldn't have it if the One was not in the semigroup.

3. partial perms and semigroups of partial perms had methods for Zero,
which according to the manual "return the additive neutral element of
the additive element". Since partial perms are not additive these
methods were inappropriate. As such they are removed in this PR and
methods for MultiplicativeZero are introduced instead.

This commit fixes these issues.
@james-d-mitchell james-d-mitchell changed the title WIP Fix One/Zero/MultiplicativeZero for partial perms Fix One/Zero/MultiplicativeZero for partial perms Dec 23, 2016
@fingolfin fingolfin merged commit 4322291 into gap-system:master Jan 3, 2017
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.0 milestone Jan 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants