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

Improve bicoset code #2686

Merged
merged 1 commit into from
Aug 9, 2018
Merged

Improve bicoset code #2686

merged 1 commit into from
Aug 9, 2018

Conversation

fingolfin
Copy link
Member

  • 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

This addresses all my remarks on PR #2666 plus some more.

- 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 fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: tests issues or PRs related to tests release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Aug 8, 2018
@codecov
Copy link

codecov bot commented Aug 8, 2018

Codecov Report

Merging #2686 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2686      +/-   ##
==========================================
+ Coverage   75.41%   75.42%   +<.01%     
==========================================
  Files         478      478              
  Lines      241590   241590              
==========================================
+ Hits       182201   182213      +12     
+ Misses      59389    59377      -12
Impacted Files Coverage Δ
lib/csetgrp.gi 63.85% <100%> (+2.11%) ⬆️
hpcgap/lib/hpc/stdtasks.g 63.42% <0%> (-1.28%) ⬇️

@@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@markuspf markuspf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 fingolfin merged commit 5aaea7a into gap-system:master Aug 9, 2018
@fingolfin fingolfin deleted the mh/bicoset branch August 9, 2018 20:33
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Sep 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes topic: tests issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants