Improve bicoset code #2686
Improve bicoset code #2686
Conversation
- add `bicoset` to the reference manual index
- clarify documentation for `IsBiCoset` ("for example" -> "if and only if")
- improve `IsBiCoset` by replacing `r*x/r` with `x^r`
- improve multiplication of right cosets to only transfer knowledge about
whether this is a bicoset if this was already known for the right operand,
instead of unconditionally computing it; but also transfer if it is not
(a bicoset times a non-bicoset is not a bicoset)
- replace `TryNextMethod` in multiplication method for right cosets with
helpful error message, just like we do in the inversion method
- improve error message when inverting a right coset which is not a bicoset
- add a test that ensures we indeed produce an error when multiplying or
inverting right cosets for which these operations are not defined
fingolfin
added
kind: enhancement
Codecov Report
@@ Coverage Diff @@
## master #2686 +/- ##
==========================================
+ Coverage 75.41% 75.42% +<.01%
==========================================
Files 478 478
Lines 241590 241590
==========================================
+ Hits 182201 182213 +12
+ Misses 59389 59377 -12
|
| @@ -667,11 +667,11 @@ function(a,b) | |||
| local c; | |||
| if ActingDomain(a)<>ActingDomain(b) then TryNextMethod();fi; | |||
| if not IsBiCoset(a) then # product does not require b to be bicoset | |||
| TryNextMethod(); | |||
| ErrorNoReturn("right cosets can only be multiplied if the left operand is a bicoset"); | |||
There was a problem hiding this comment.
Making this an error will hard block any future attempt to define multiplication of ordinary cosets (e.g. as multiplication of sets) in a more general way through other methods. Is this deliberate?
There was a problem hiding this comment.
This was quite deliberate, i.e., I thought quite a while about whether to change this into an error; or whether to instead change the Inverse method to use TryNextMethod() instead of Error -- in any case, I think they should match.
However, in the end I think a clear helpful error message is far better than a "method not found", just to support a hypothetical future extension which may never come. Besides, we actually don't stop anybody from adding one: they just have to rank their method higher than ours (and make sure to stay compatibly / invoke TryNextMethod() if needed). Finally, we can of course always change the error into something else if it ever becomes necessary.
There was a problem hiding this comment.
I am ambivalent on @hulpke's remark:
I think the new error message is clearer than a potential "no method found" or (worse) wrong result because some method is selected down the line. The code can always be changed in future with the minor inconvenience to maybe having to figure out where;
Of course we could install more precise methods for IsRightCoset and IsBiCoset, but that does not improve error messages as things stand, either.
fingolfin
added
release notes: added
bicosetto the reference manual indexIsBiCoset("for example" -> "if and only if")IsBiCosetby replacingr*x/rwithx^rTryNextMethodin multiplication method for right cosets with helpful error message, just like we do in the inversion methodThis addresses all my remarks on PR #2666 plus some more.