FIX/ENHANCE: Small fixes and corrections, staged for 4.9.1 #2035
Conversation
hulpke
added
the
kind: enhancement
FIX/ENHANCE: ImageKernelBlockHomomorphism memory and performance. Merged now as all tests work fine and it can clearly go into master, there is a shadow commit #2035 to add the code into 4.9.1 once 4.9.0 is done.
Codecov Report
@@ Coverage Diff @@
## stable-4.9 #2035 +/- ##
==============================================
+ Coverage 69.19% 69.22% +0.03%
==============================================
Files 493 493
Lines 259991 260030 +39
==============================================
+ Hits 179900 180008 +108
+ Misses 80091 80022 -69
|
There was a problem hiding this comment.
#2025 with same changes has been merged into master too fast - it occurs that it now runs out of memory there if all packages are loaded. Please do not merge this until further investigation. This PR is assigned to GAP 4.9.1, so it should wait at least till GAP 4.9.0 will be out.
|
It seems either I have misunderstood the branching process or not explained clearly what this PR does vs. #2025: The stable-4.9 branch has been forked off and will go to become the 4.9 releases, so I think this change is substantial enough in effect and harmless enough in actual change that it should go into 4.9, but with packing I understand if it should only go into 4.9.1 This PR is to merge the same changes there. I do not intend to merge it before 4.9.0 is done. |
olexandr-konovalov
changed the title
The old code added every generator found with `AddGeneratorsExtendSchreierTree'. This caused the storage of lots of generators and subsequent memory issues. Also, if there are few large blocks, Butler's block homomorphism code is inefficient and it needs to run through all points in one block. In this case an ordinary orbit calculation is far more efficient. Together these issues did cause severe problems in large degrees with few blocks (observed by Thomas). It it possible that it also rectifies some of the observed problems in larger degree. What one could still do is to use random subproducts instead of testing all Schreier generators, but this is not the right time to do so.
hulpke
changed the title
hulpke
changed the title
hulpke
dismissed
olexandr-konovalov’s stale review
I'm dismissing the review (in part to see what this does and) as it is not about the contents but only about not committing before 4.9.1 (which the title and milestone state). This also should make it clear that the changes still require a proper content review and do not have content changes pending.
as requested in gap-system#2131 Also added `LowLayerSubgroups` and minor rephrasing. Also redid part of gap-system#683, as for some reason (probably my stupidity) part of gap-system#683 had fallen out of master. Added it again.
olexandr-konovalov
added
the
do not merge
|
PLEASE BE AWARE: these changes are already in master branch and are causing running out of memory in |
There was a problem hiding this comment.
Please be advised: For ongoing development of the 4.9.x branch, we (@ChrisJefferson @alex-konovalov @markuspf and myself) to change how we work: Instead of making changes on the stable branch, and then merging the stable-4.9 branch regulalry back into master, we will instead continue with PRs exclusively to the master branch. Then, any changes which should also go into stable-4.9 will be cherry-picked there (possibly via an intermediate PR).
The motivation for this is that this way, one can proceed making changes on master quickly, and does not have to wait for the changes to slowly trickle back from stable-4.9 into master. Moreover, fewer people have to worry about how to get changes "correctly" into stable-4.9: Just submit a PR against master, and indicate that you'd like the changes in there to also go into stable-4.9, and we'll take care of it.
Moreover, we expect stable-4.9 to be really treated as stable: I.e. no new developments should happen here, only strict bug fixes.
As it, this PR contains both new changes (e.g. to the documentation) which are not in master nor in stable-4.9. I have opened PR #2158 against the master branch with the new changes. Once that is merged, we can either merge this PR #2035 here, or perhaps replace it by an equivalent fresh PR full of cherry-picks (I could make it, no need to waste @hulpke's time).
Of course we can also do things differently, and e.g. @hulpke creates his own equivalent of my PR #2158, or yet something else; this all is not set in stone, and is not intended to cause extra work for @hulpke , so I hope we can work together to find a good solution.
|
@fingolfin |
|
Sorry for the confusion, I meant to review #2057 and then got confused by too many open browser tabs sigh. Anyway, this PR is alone one where I totally missed an update. I'll now look into this PR, too, to figure out where we add, and will comment again later. |
|
@fingolfin |
|
I now made PR #2253 which contains the same changes as in this PR, but freshly cherry-picked from master, so that they are really identical (a few of the manual changes differed between this PR and master). As to marking commits for backporting: Yeah, I'd include a fixed string somewhere in the commit message. E.g. "Should be backported to stable-4.9" ? I'll open an issue for this so others can join the discusson. |
fingolfin
added
release notes: added
Includes #2025
Fix for #2055. #2104, #2131 (as far as LowIndex is concerned)
Multiple documenttion/testfile/formatting fixes.