FIX: Add IsBiCoset #2666

hulpke · 2018-07-28T21:50:31Z

and allow coset multiplication only for bicosets (as otherwise its not well-defined.

This fixes #2555

There is no test for the condition of #2555, as the illegal coset multiplication now runs into a `no method found' error.

and allow coset multiplication only for bicosets (as otherwise its not well-defined. This fixes gap-system#2555

markuspf · 2018-07-31T08:09:56Z

lib/csetgrp.gd

+##  <Prop Name="IsBiCoset" Arg='C'/>
+##
+##  <Description>
+##  A (right) cose is considered a <E>bicoset</E> if its set of elements simultaneously forms a left


Typo: coset

markuspf · 2018-07-31T08:15:41Z

Just as a remark (not strictly required to merge but desirable) we can test this behaviour with the following test file

gap> G := SymmetricGroup(3);;
gap> U := Group( (1,2) );;
gap> cos := RightCosets(G,U);;
gap> cos[2]*cos[2];
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 2nd choice method found for `*' on 2 arguments

#

fingolfin

This seems fine in principle, just some remarks. And I don't understand this:

There is no test for the condition of #2555, as the illegal coset multiplication now runs into a `no method found' error.

That's not a problem for adding a test, as also illustrated by @markuspf; since this PR is about fixing that bug, I'd really wish there was a test for this. But I'll survive either way shrug

fingolfin · 2018-07-31T17:01:42Z

lib/csetgrp.gd

+##
+##  <Description>
+##  A (right) coset is considered a <E>bicoset</E> if its set of elements simultaneously forms a left
+##  coset for the same subgroup. This is the case, for example, if the coset representative normalizes


Why "for example"? This is an equivalence, isn't it?

fingolfin · 2018-07-31T17:04:02Z

lib/csetgrp.gi

+local s,r;
+  s:=ActingDomain(c);
+  r:=Representative(c);
+  return ForAll(GeneratorsOfGroup(s),x->r*x/r in s);


Perhaps test for x^r in s instead, as that will be a bit more efficient in some cases (e.g. for permutations)?

Yes, if this ever could be time-critical this has the potential to be faster. I decided on the inverse-conjugation as it translated directly to the condition as I had written it doen on paper.

fingolfin · 2018-07-31T17:05:54Z

lib/csetgrp.gi

+    TryNextMethod();
+  fi; 
+  c:=RightCoset(ActingDomain(a), Representative(a) * Representative(b) );
+  if IsBiCoset(b) then


Perhaps first check HasIsBicoset to avoid a computation?

fingolfin · 2018-07-31T17:06:53Z

lib/csetgrp.gi

@@ -668,10 +683,12 @@ function(a)
 local s,r;
  s:=ActingDomain(a);
  r:=Representative(a);
-  if ForAny(GeneratorsOfGroup(s),x->not x^r in s) then
+  if not IsBiCoset(a) then
    Error("Inversion only works for cosets of normal subgroups");


That error message is wrong (though of course it was before, too)

fingolfin · 2018-08-01T11:00:26Z

@hulpke to clarify (esp. in light of what was discussed during yesterdays video conference call; you may have read the notes): all my comments are about minor issues. If you want, you can ignore them, and just merge this, though personally I think it would make sense to at least add a test as suggested by @markuspf. I can address any remaining points with a PR of my own afterwards, too (or alternatively, I can also push fixes to this PR, if you are fine with that)

hulpke · 2018-08-02T16:22:47Z

@fingolfin
Thank you for your last remark. Yes, these are small changes (and I don't mind if someone else does these changes, in whatever format is easiest to do.

What can be awkward awkward (which is not the case here) is the combination of a slew of minor suggestions with the required approval of a PR, which can look as if a moving hurdle is put up higher and higher, requiring the justification or change of ad-hoc decisions.

As for testing for a hard error, this will require a change if cosets ever get interpreted as sets of elements and get set-wise multiplication. As I also did not know how to do such a check in a test file, it seemed safer to leave it out.

hulpke · 2018-08-02T16:39:01Z

Ignore the close/open. Clumsy fingers when carrying the laptop with the web page open...

markuspf · 2018-08-02T21:54:38Z

Just to clarify: Do we want to merge this now?

hulpke · 2018-08-07T19:12:49Z

@markuspf I wasn't planning to add more, so as far as I'm concerned this is ready to merge or to discard.

FIX: Add IsBiCoset

45f14a7

and allow coset multiplication only for bicosets (as otherwise its not well-defined. This fixes gap-system#2555

hulpke added do not comment yet PRs on which the author does not yet want any comment (e.g. only submitted for test results) and removed do not comment yet PRs on which the author does not yet want any comment (e.g. only submitted for test results) labels Jul 28, 2018

hulpke requested a review from fingolfin July 30, 2018 19:52

markuspf reviewed Jul 31, 2018

View reviewed changes

markuspf added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them labels Jul 31, 2018

REBASE: typo

9d4cc52

fingolfin approved these changes Jul 31, 2018

View reviewed changes

hulpke closed this Aug 2, 2018

hulpke reopened this Aug 2, 2018

markuspf merged commit 3681bec into gap-system:master Aug 7, 2018

fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Aug 8, 2018

fingolfin mentioned this pull request Aug 8, 2018

Improve bicoset code #2686

Merged

hulpke deleted the ah/bicoset branch September 27, 2018 16:01

fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Sep 28, 2018

fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Add IsBiCoset #2666

FIX: Add IsBiCoset #2666

hulpke commented Jul 28, 2018

markuspf Jul 31, 2018

markuspf commented Jul 31, 2018 •

edited

Loading

fingolfin left a comment

fingolfin Jul 31, 2018

fingolfin Jul 31, 2018

hulpke Aug 2, 2018

fingolfin Jul 31, 2018

fingolfin Jul 31, 2018

fingolfin commented Aug 1, 2018

hulpke commented Aug 2, 2018 •

edited

Loading

hulpke commented Aug 2, 2018

markuspf commented Aug 2, 2018

hulpke commented Aug 7, 2018

FIX: Add IsBiCoset #2666

FIX: Add IsBiCoset #2666

Conversation

hulpke commented Jul 28, 2018

markuspf Jul 31, 2018

Choose a reason for hiding this comment

markuspf commented Jul 31, 2018 • edited Loading

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Jul 31, 2018

Choose a reason for hiding this comment

fingolfin Jul 31, 2018

Choose a reason for hiding this comment

hulpke Aug 2, 2018

Choose a reason for hiding this comment

fingolfin Jul 31, 2018

Choose a reason for hiding this comment

fingolfin Jul 31, 2018

Choose a reason for hiding this comment

fingolfin commented Aug 1, 2018

hulpke commented Aug 2, 2018 • edited Loading

hulpke commented Aug 2, 2018

markuspf commented Aug 2, 2018

hulpke commented Aug 7, 2018

markuspf commented Jul 31, 2018 •

edited

Loading

hulpke commented Aug 2, 2018 •

edited

Loading