Reduce impact of immediate methods: change some to implications, remove others, use custom SetSize method (rebased #2387) #2522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit changes routines that create groups, certain Submagmas and vector spaces so that flags that set basic properties (being trivial, being empty, being cyclic) are set at the same time as the object was created. This functionality used to be provided through immediate methods, however this meant that the type of a just-created object was changed immediately multiple times (with some changes precipitating other changes), causing a notable performance hit. Notably, the following changes have been done: 1) In a number of methods used to create groups (or submagmas, when called from `Subgroup` or cosets) from generating sets, a number of cheaply deduced properties (such as being cyclic) are set while creating the object. Thus the immediate methods do not apply for these objects any longer. 2) Type caching in these methods is not worth the effort and has been removed. 3) A new setter method for `Size` deals similarly with deductions done before by immediate methods for a known size. Caveat: The manual specifies (ill-advised one might say) that setting a different size if a size already is given will be ignored. This is tested in manual examples and test files. The new setter method therefore cannot be used to check for this property as an error -- it will do so only if assertion level is set >=3. 4a) Immediate methods that have been made obsolete by the changes in 1) and 3) have been changed to ordinary methods. 4b) A number of immediate methods (e.g. a trivial group is solvable) have been replaced by `TrueMethods` that have the same effect but are chapter to run. 4c) Also a number of immediate methods that need to run often, but apply only rarely (i.e. setting `IsFinite` to false if a size is set to `infinity`) have been removed. 5) These changes have minor impact on test files and manual tests: A few objects will have slightly different knowledge about their properties and thus print differently; respectively test code exists that checks explicitly for such properties having been set by immediate methods. These tests and examples have been changed. A few, very unstable tests (e.g. checking explicitly for the values of random elements) have been commented out. 6) Some tests explicitly relied on immediate methods enabling derived series calculations in special cases of finitely presented groups. These tests never would have worked if immediate methods were turned off, the respective finitely presented groups method has been changed accordingly to test explicitly for the particular situation, thus enabling the same examples to work.
fingolfin
added
the
kind: enhancement
fingolfin
requested review from
ChrisJefferson,
markuspf,
stevelinton,
frankluebeck and
hulpke
fingolfin
changed the title
fingolfin
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2522 +/- ##
==========================================
- Coverage 74.34% 74.33% -0.01%
==========================================
Files 481 481
Lines 243407 243428 +21
==========================================
+ Hits 180955 180964 +9
- Misses 62452 62464 +12
|
hulpke
approved these changes
Jun 7, 2018
ChrisJefferson
approved these changes
Jun 7, 2018
stevelinton
approved these changes
Jun 7, 2018
olexandr-konovalov
added
the
gapdays2018-spring
fingolfin
added
the
release notes: added
ThomasBreuer
added a commit
to ThomasBreuer/gap
that referenced
this pull request
Dec 5, 2018
change the default methods for `GroupByGenerators` and `GroupWithGenerators` such that the problem gets fixed that had been introduced with pull request gap-system#2522 and discussed in issue gap-system#2703
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a rebased version of #2387, which resolves a conflict, fixes some typos in the commit message, and adds a few more related changes of mine in two separate commits.
Assuming the tests pass, I plan to merge this tomorrow (since I am in St Andrews, I might talk some people here into looking at it first, too ;-).
@hulpke sorry for the delays :/
Closes #2387