-
Notifications
You must be signed in to change notification settings - Fork 161
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
Install {8,16,32}Bits_Quotient kernel funcs as methods #2616
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2616 +/- ##
==========================================
- Coverage 74.81% 74.8% -0.01%
==========================================
Files 479 479
Lines 242190 242190
==========================================
- Hits 181184 181176 -8
- Misses 61006 61014 +8
|
@hulpke This is the alternative PR which tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment which I had added to the first relevant method installation holds also for the other method installations.
As soon as this comment has been addressed, I will approve the pull request.
lib/wordrep.gi
Outdated
"for two 8 bits assoc. words", | ||
IsIdenticalObj, | ||
[ Is8BitsAssocWord, Is8BitsAssocWord ], 0, | ||
8Bits_Quotient ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method would be applicable also to elements of, for example, FreeMonoid( IsSyllableWordsFamily, 2 )
,
with the effect that corrupted objects arise.
If IsMultiplicativeElementWithInverse
is required for the second argument then this cannot happen,
and one can use InstallMethod
instead of InstallOtherMethod
.
916c40d
to
7464992
Compare
@ThomasBreuer Thank you! I just updated the PR based on your comments |
I think we should keep the functions and install them as methods, i.e. I prefer this PR over #2615 |
This is an alternative to #2615.
Note that some benchmarks indicate that these kernel functions do provide a noticeable speed up: (here
x32
andu32
are defined intst/testinstall/wordrep.tst
):But I wonder if there is a reason they were not used?
Resolves #2504. Closes #2615.