-
Notifications
You must be signed in to change notification settings - Fork 160
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
Avoid some integer factoring; use PrimeDivisors #2094
Conversation
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.
Very good. The PrimeDivisors
was on my to do list, even better its done.
The weird Yndex
(which was written by Heiko Theissen) routine predates the quick recognition of S_n. Index
would have called Size
which would have called a stabilizer chain , which for a large S_n is cumbersome. There is no need for it any longer.
Codecov Report
@@ Coverage Diff @@
## master #2094 +/- ##
==========================================
+ Coverage 69.27% 69.32% +0.05%
==========================================
Files 491 491
Lines 257326 258098 +772
==========================================
+ Hits 178269 178937 +668
- Misses 79057 79161 +104
|
9c890cc
to
a097e58
Compare
@frankluebeck said in #2087:
Should this PR use |
This PR does not use |
Aside from this, admittedly pertinent, point though, the answer is yes. If you know the ring you want to factor over, it is always better to provide it. Apart from performance concerns, GAP will sometimes not default to the ring you expect. |
Also remove the commented out
YndexSymmetricGroup
(it contained a call toFactorsInt
), and replaceLength(Filtered(...))
byNumber(...)
(motivation: such code was close to code callingFactorsInt
).This is motivated by PR #2087. Note that
PrimeDivisors
currently callsFactorsInt
, but now this is in a single place.