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

Avoid some integer factoring; use PrimeDivisors #2094

Merged
merged 4 commits into from
Jan 20, 2018

Conversation

fingolfin
Copy link
Member

Also remove the commented out YndexSymmetricGroup (it contained a call to FactorsInt), and replace Length(Filtered(...)) by Number(...) (motivation: such code was close to code calling FactorsInt).

This is motivated by PR #2087. Note that PrimeDivisors currently calls FactorsInt, but now this is in a single place.

@fingolfin fingolfin added topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Jan 18, 2018
Copy link
Contributor

@hulpke hulpke left a 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
Copy link

codecov bot commented Jan 19, 2018

Codecov Report

Merging #2094 into master will increase coverage by 0.05%.
The diff coverage is 68.36%.

@@            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
Impacted Files Coverage Δ
lib/stbcbckt.gi 84.41% <ø> (+0.05%) ⬆️
lib/autsr.gi 0% <0%> (ø) ⬆️
lib/randiso.gi 26.11% <0%> (ø) ⬆️
lib/upoly.gi 57.84% <0%> (ø) ⬆️
lib/ctblfuns.gi 60.21% <0%> (ø) ⬆️
lib/gprd.gi 49.62% <0%> (ø) ⬆️
lib/schur.gi 19.33% <0%> (ø) ⬆️
lib/grppcaut.gi 44.51% <0%> (+0.23%) ⬆️
lib/gaussian.gi 69.31% <100%> (ø) ⬆️
lib/polyfinf.gi 86.63% <100%> (ø) ⬆️
... and 101 more

@olexandr-konovalov
Copy link
Member

@frankluebeck said in #2087:

A call of Factors(Integers, x) instead of just Factors(x) can give a significant speedup if this is called frequently with small x.

Should this PR use Factors(Integers, x) instead of Factors(x) where possible then?

@fingolfin
Copy link
Member Author

This PR does not use Factors, so: no.

@ChrisJefferson ChrisJefferson merged commit 97ce295 into gap-system:master Jan 20, 2018
@fingolfin fingolfin deleted the mh/no-factors branch January 20, 2018 17:00
@stevelinton
Copy link
Contributor

@alex-konovalov @fingolfin

Should this PR use Factors(Integers, x) instead of Factors(x) where possible then?

This PR does not use Factors, so: no.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants