Reorder methods after new implications are added #2773
Merged
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
fingolfin
force-pushed
the
mh/reorder-filters
branch
from
7d7dcd8
to
ea8f7d8
Compare
Codecov Report
@@ Coverage Diff @@
## master #2773 +/- ##
=========================================
Coverage ? 75.92%
=========================================
Files ? 481
Lines ? 241925
Branches ? 0
=========================================
Hits ? 183679
Misses ? 58246
Partials ? 0
|
fingolfin
force-pushed
the
mh/reorder-filters
branch
2 times, most recently
from
fa00be9
to
44a8458
Compare
fingolfin
requested review from
stevelinton,
frankluebeck,
ChrisJefferson,
ThomasBreuer and
markuspf
fingolfin
added
kind: bug
ThomasBreuer
requested changes
Sep 12, 2018
fingolfin
force-pushed
the
mh/reorder-filters
branch
from
44a8458
to
01f8b25
Compare
ThomasBreuer
approved these changes
Sep 12, 2018
Many GAP methods get rank adjustments based on the rank of some filter, e.g, `RankFilter(IsAbelian)`. However, these ranks may change when new implications are added, e.g. when certain packages are loaded. But those method rank adjustments then won't be updated, which can ultimately lead to the effect that loading a package changes which methods get executed in otherwise identical code. This PR helps avoid that, by making it possible to set "dynamic" rank adjustments which get updated as new implications are added. Some cleverness is needed to make this work efficiently when loading packages. - Record rank adjustment in METHODS_OPERATION - Add RECALCULATE_ALL_METHOD_RANKS function which does what it says. - Adjust MethodsOperation to include base rank - Increase size of HIDDEN_IMPS_CACHE and WITH_IMPS_CACHE to speed up RECALCULATE_ALL_METHOD_RANKS() - Make sure RANK_FILTERS is always set promptly, then remove the test for it being unset in RankFilter - Automatically reorder methods after library or package loading, or InstallTrueMethod - Use COPY_LIST_ENTRIES in method installation to avoid making an intermediate list - Support passing a function of no arguments as the rank adjustment to InstallMethod - Reset reordering status on quit from break loop. Fixes gap-system#2513
fingolfin
force-pushed
the
mh/reorder-filters
branch
from
01f8b25
to
17d1bac
Compare
... to use a rank *function* instead of a single fixed rank value.
|
No problem, thanks for carefully reading! I removed the comma. In addition, I added a commit which converts a bunch of A similar thing likely should be done in pacakges... Now it would be really good if @stevelinton could comment how he'd like to proceed... |
|
I'm happy with this. The older PR kept getting test failures which, I think, were actually not caused by the PR but by test infrastructure glitches. Otherwise I'd have merged it. |
This was referenced Sep 13, 2018
|
This has passed all tests and been extensively reviewed. Time to merge. |
This was referenced Sep 16, 2018
fingolfin
changed the title
fingolfin
added
release notes: added
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 rebased and cleaned up version of #2521; I removed many whitespace-only changes, reformatted a few things and otherwise squashed most changes into a single commit.
Closes #2521
Fixes #2513