Reorder methods after new implications are added #2521

stevelinton · 2018-06-06T16:40:17Z

This is the start of an attempt to address #2513. At this stage it provides a function RECALCULATE_ALL_METHOD_RANKS() which does what it says. It is not called automatically. It still has a debugging printout when method ordering changes. It only seems to break one thing in testinstall

codecov · 2018-06-07T10:42:16Z

Codecov Report

❗ No coverage uploaded for pull request base (master@64f4323). Click here to learn what that means.
The diff coverage is 94.46%.

@@            Coverage Diff            @@
##             master    #2521   +/-   ##
=========================================
  Coverage          ?   75.42%           
=========================================
  Files             ?      478           
  Lines             ?   241759           
  Branches          ?        0           
=========================================
  Hits              ?   182343           
  Misses            ?    59416           
  Partials          ?        0

Impacted Files	Coverage Δ
src/opers.c	`94% <ø> (ø)`
lib/init.g	`71.76% <ø> (ø)`
lib/error.g	`38.2% <0%> (ø)`
src/hpc/c_type1.c	`82.76% <100%> (ø)`
src/c_type1.c	`83.41% <100%> (ø)`
lib/package.gi	`69.82% <33.33%> (ø)`
lib/filter.g	`92.85% <83.33%> (ø)`
lib/oper.g	`82.23% <92.47%> (ø)`
src/hpc/c_oper1.c	`87.11% <98.91%> (ø)`
src/c_oper1.c	`88.31% <99.14%> (ø)`

fingolfin · 2018-06-07T10:55:31Z

lib/oper1.g

+        ranknow := rank();
+    else 
+        ranknow := rank;
+    fi;    


Perhaps rename rank to baserank, and ranknow to rank, in order to reduce the diffs?

stevelinton · 2018-06-07T14:59:40Z

So the problem (or at least the first one) turns out to be for IsomorphismPermGroup where the method for permutation groups overtakes the method for finite nilpotent groups. I'm not sure why exactly this causes the computation to fail, and that is almost certainly a bug that needs fixing.

stevelinton · 2018-06-08T14:24:30Z

The bug that arose is fixed in #2533 (the reordering revealed a real bug in the library). There are still a few changes to output formats and similar

stevelinton · 2018-06-13T13:14:29Z

This is now working, as far as I can see, although the diagnostic printout needs to be removed before it is merged. The method rankings are recomputing to respect current implications:

when the library and its needed/recomended packages are loaded and before reading files from the command-line
after loading a package (except that LOAD_ALL_PACKAGES only does it once at the end)
any other time when the implications caches are being refreshed

This takes about 60ms (on my laptop) in a standard GAP session if there is nothing to do (I had to expand a couple of caches to get it down to that) but about 1.5second with all packages (more or less) loaded.

Documentation and possibly tests are also still due.

fingolfin · 2018-06-13T15:09:28Z

Tests fail: first off, make docomp needs to be run in HPC-GAP, too (it has its own set of c_*.c files). Secondly, testspecial is broken, because those tests "see" all those debug outputs.

fingolfin · 2018-06-15T11:24:09Z

lib/package.gi

+        else
+            List( RecNames( GAPInfo.PackagesInfo ), LoadPackage );
+        fi;	
+        ResumeMethodReordering();        


This change of indentation increases the diff needlessly... It seems the only actual changes was that a call to SuspendMethodReordering was inserted?

Oh and also ResumeMethodReordering

Thanks, will fix.

fingolfin · 2018-06-15T11:28:09Z

lib/oper.g

+                    continue;
+                fi;
+                meths[base+n+3] := rank;
+                if i = 1 or rank <= meths[base-3] then


Why -3? Wait, is that trying to access the previous method's entry? But that could break if BASE_SIZE_METHODS_OPER_ENTRY changes again, no? It's also not immediately clear which "field" base-3 refers to. While this makes it clear: meths[base+n+3 - BASE_SIZE_METHODS_OPER_ENTRY] (I'd still ad a comment to explain what's going on, though?)

fingolfin · 2018-06-15T11:29:40Z

doc/ref/methsel.xml

@@ -252,6 +252,7 @@ one can install also <E>immediate methods</E>.
 <Heading>Logical Implications</Heading>

 <#Include Label="InstallTrueMethod">
+<#Include Label="MethodReordering">


Do we really want to make this publicly documented? Why?

fingolfin · 2018-06-15T11:31:55Z

lib/filter.g

+##
+#F SuspendMethodReordering()
+#F ResumeMethodReordering()
+#F ResetMethodReordering()


For consistency with other files, there should be two spaces after each #F, and also () should be ( ) (but we'll survive w/o this changed, too ;-)

fingolfin · 2018-06-15T11:32:02Z

lib/filter.g

+##  <Func Name="ResumeMethodReordering" Arg=""/>
+##  <Func Name="ResetMethodReordering" Arg=""/>
+##
+##  <Description> These functions control whether the method reordering process


Virtually every other GAPDoc comment in the library has <Description> and </Description> on their own lines. Just sayin'

fingolfin · 2018-06-15T11:33:06Z

lib/filter.g

+##  described in <Ref Func="InstallTrueMethod"/> is invoked or not. Since this 
+##  process can be comparatively time-consuming, it is usually suspended when
+##  a lot of implications are due to be installed, for instance when loading
+##  the library, or a package. This is done by called <C>SuspendMethodReordering()</C>


called -> calling? Also, the recommend usage is that every reference to a function should use <Ref Func=...>, even if it is inside the body of the documentation of the function itself.

fingolfin · 2018-06-15T11:36:34Z

lib/filter.g

+##
+##  <C>ResetMethodReordering()</C> effectively exits all nested suspensions, resuming
+##  reordering immediately. This function is mainly provided for error recovery and 
+##  similar purposes.</Description>


Indeed, what if there is an error duing package loading? That happens frequently for me if I try LoadAllPackages() (because I have tons of non-distributed packages and development versions, with all kinds of conflicts). For options, we have the OnQuit hook. Perhaps a call to ResetMethodReordering should be added to OnQuit, too? (Both in the GAP and in the HPC-GAP version).

fingolfin · 2018-06-15T11:37:48Z

lib/filter.g

+##  By default <C>InstallTrueMethod</C> adjusts the method selection data structures 
+##  to take care of this, but this process can be time-consuming, so functions
+##  <Ref Func="SuspendMethodReordering"/> and <Ref Func="ResumeMethodReordering"/>
+##  are provided to allow control of this process.


I think it's good to document this somewhere. But do we really want to document it for the public? I.e. perhaps move this out of the GAPDoc comment; or put it inside an XML comment?

fingolfin · 2018-06-15T11:38:05Z

lib/filter.g

 ##  </Description>
 ##  </ManSection>
 ##  <#/GAPDoc>
 ##
+
+


Why insert these three empty lines?

fingolfin · 2018-06-15T11:38:41Z

lib/oper.g

@@ -1922,6 +1917,96 @@ BIND_GLOBAL( "FLUSH_ALL_METHOD_CACHES", function()
 end);

 fi;
+#############################################################################


Insert an empty line before this comment starts?

fingolfin · 2018-06-15T11:43:34Z

src/opers.c

@@ -1946,6 +1946,9 @@ static ALWAYS_INLINE Obj GetMethodUncached(
        // entry n+3 is the rank
        // entry n+4 is the info text
        // entry n+5 is, if set, the location where the method was installed
+        // entry n+6 is, if set, the relative rank that was supplied when
+        //                       the method was installed, either as a small integer or a function
+        //                       of no arguments


That's a somewhat odd wrapping (looking at the differing line lengths); also, could perhaps align at "if set"?

And perhaps should say: "a function of no arguments, which returns a small integer to be used as rank"?

This use of such a function as "base rank" should ideally be documented somewhere, too, no? And I guess we'd also have to go through the library to search for places where this should be used...

fingolfin · 2018-06-15T11:47:47Z

tst/testinstall/method-reordering.tst

+
+
+
+


You may need to remove these trailing empty lines, at least if/once this is rebased on master (as we added tests for "malformed" test files -- not sure if they accept trailing empty lines... perhaps they do, after all)

stevelinton · 2018-06-15T12:00:55Z

@fingolfin thanks for the review. I'll pick up the small things after I've fixed constructors and actually supported the function for the rank adjustment in InstallMethod (which I've just realised I need to do).

If we're going to do this, I do think it should be documented publicly (although it doesn't need a huge song and dance) because there is a small but real risk of two things:

Package code or future library development installs implications which change the ranking of existing library methods causing surprises
Some package (homalg -- I'm looking at you) turns out to install lots of implications at "run time" after package loading and suddenly gets a lot slower.

In either case I think it's better to be able to point people at documentation.

stevelinton · 2018-06-28T15:46:58Z

Subject to the tests, I think this is now complete.

fingolfin · 2018-07-06T13:34:32Z

@stevelinton I fixed #2594 in PR #2604, so if approve and merge the latter, it should be possible to resume with this PR

stevelinton · 2018-07-24T14:54:19Z

The only test now failing is testpackages and that seems to be failing to loaf float, which loads OK for me on my laptop in this branch. Anyone know what's happening.

olexandr-konovalov · 2018-07-24T16:01:33Z

We seen this in float before, I am sure it's not related to this PR:

Loading float ... 
#W dlopen() error: /home/travis/build/gap-system/gap/pkg/float-0.9.1/bin/x86_6\
4-pc-linux-gnu-default64/float.so: undefined symbol: atexit
Error, module '/home/travis/build/gap-system/gap/pkg/float-0.9.1/bin/x86_64-pc-linux-\
gnu-default64/float.so' not found in
  if not LOAD_DYN( arg[1], false ) then
    Error( "no support for dynamic loading" );
fi; at /home/travis/build/gap-system/gap/lib/files.gd:583 called from

I've restated it so see if this will stay - someone suggested that this may depend on underlying Travis CI setups, and does not happen each time.

(Cf. #2391).

stevelinton · 2018-08-01T14:33:25Z

I'm completely mystified by the test failure. float won't load because "atexit" is not defined, but (a) it's not actually mentioned anywhere in float that I can see and (b) it's a standard library function. I also don't see how the changes in this PR can possibly be related.

fingolfin · 2018-08-01T14:48:48Z

This is a problem with float, or rather, the binutils/linker version installed on Travis. It affected many PRs and also master, hence we disabled float in the tests now. If you rebase on latest master, this should go away

Adjust MethodsOperation to include base rank

…CALCULATE_ALL_METHOD_RANKS()

…or it being unset in RankFilter

…stallTrueMethod

…mediate list

…InstallMethod

fingolfin · 2018-08-21T21:01:08Z

@stevelinton any plans to rebase this (and perhaps squash some of the comments)?

fingolfin · 2018-09-04T15:10:56Z

@stevelinton ping?

fingolfin · 2018-09-04T15:36:37Z

Since I'd like to make some more changes in lib/oper.g, it would be good to finalize this. In order to make progress, I will push a cleaned up and rebased version of this PR as a fresh PR (if @stevelinton agrees, I can also later update this PR here instead, but for now, I don't want to modify his work w/o permission)

fingolfin · 2018-09-04T22:32:23Z

See PR #2773

stevelinton added kind: bug Issues describing general bugs, and PRs fixing them do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) kind: discussion discussions, questions, requests for comments, and so on labels Jun 6, 2018

stevelinton force-pushed the reorder-filters branch from edd1329 to d05e382 Compare June 7, 2018 10:42

fingolfin reviewed Jun 7, 2018

View reviewed changes

stevelinton mentioned this pull request Jun 7, 2018

Method order does not change when filter ranks change, leading to subtle conflict between nq and lpres #2513

Closed

stevelinton force-pushed the reorder-filters branch from d05e382 to 0a43914 Compare June 8, 2018 09:19

olexandr-konovalov added the gapdays2018-spring Issues and PRs that arose at http://www.gapdays.de/gap-jupyter-days2018 label Jun 8, 2018

stevelinton mentioned this pull request Jun 10, 2018

Don't use fast method for composing with identity mapping if source & range don't match #2533

Merged

stevelinton force-pushed the reorder-filters branch 4 times, most recently from d6d0d66 to ae15f65 Compare June 13, 2018 13:09

stevelinton force-pushed the reorder-filters branch from ae15f65 to 64b75ff Compare June 14, 2018 11:05

fingolfin reviewed Jun 15, 2018

View reviewed changes

stevelinton force-pushed the reorder-filters branch 2 times, most recently from 43a97b4 to 3565b1d Compare June 15, 2018 11:44

fingolfin reviewed Jun 15, 2018

View reviewed changes

stevelinton force-pushed the reorder-filters branch 2 times, most recently from b1e63c6 to 3c8b617 Compare June 28, 2018 15:41

stevelinton removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jun 28, 2018

stevelinton force-pushed the reorder-filters branch 3 times, most recently from 40ad33c to f12b1e1 Compare July 24, 2018 13:35

stevelinton force-pushed the reorder-filters branch from f12b1e1 to 33c2729 Compare July 26, 2018 10:11

stevelinton force-pushed the reorder-filters branch from 33c2729 to 0b34b4b Compare August 1, 2018 14:52

stevelinton added 12 commits August 1, 2018 22:50

Record rank adjustment in METHODS_OPERATION

be8920b

Add RECALCULATE_ALL_METHOD_RANKS function which does what it says.

7812f31

Adjust MethodsOperation to include base rank

Increase size of HIDDEN_IMPS_CACHE and WITH_IMPS_CACHE to speed up RE…

a3cabba

…CALCULATE_ALL_METHOD_RANKS()

Make sure RANK_FILTERS is always set promptly, then remove the test f…

953bc86

…or it being unset in RankFilter

Automatically reorder methods after library or package loading, or In…

5c4a76c

…stallTrueMethod

Add test

d51f79e

Documentation

1d1279d

HPCGAP support

97cecad

Handle Constructors

533c1b6

Use COPY_LIST_ENTRIES in method installation to avoid making an inter…

d03cd97

…mediate list

Support passing a function of no arguments as the rank adjustment to …

e29f5b9

…InstallMethod

Reset reordering status on quit from break loop.

28d3d3f

stevelinton force-pushed the reorder-filters branch from 0b34b4b to 28d3d3f Compare August 1, 2018 21:50

This was referenced Sep 4, 2018

Reorder methods after new implications are added #2773

Merged

Some filter related refactoring #2775

Merged

fingolfin closed this in #2773 Sep 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorder methods after new implications are added #2521

Reorder methods after new implications are added #2521

stevelinton commented Jun 6, 2018

codecov bot commented Jun 7, 2018 •

edited

Loading

fingolfin Jun 7, 2018

stevelinton commented Jun 7, 2018

stevelinton commented Jun 8, 2018

stevelinton commented Jun 13, 2018

fingolfin commented Jun 13, 2018

fingolfin Jun 15, 2018

fingolfin Jun 15, 2018

stevelinton Jun 15, 2018

fingolfin Jun 15, 2018

stevelinton Jun 15, 2018

fingolfin Jun 15, 2018

fingolfin Jun 15, 2018

fingolfin Jun 15, 2018

fingolfin Jun 15, 2018

fingolfin Jun 15, 2018

fingolfin Jun 15, 2018

fingolfin Jun 15, 2018

fingolfin Jun 15, 2018

fingolfin Jun 15, 2018

fingolfin Jun 15, 2018

stevelinton commented Jun 15, 2018

stevelinton commented Jun 28, 2018

fingolfin commented Jul 6, 2018

stevelinton commented Jul 24, 2018

olexandr-konovalov commented Jul 24, 2018 •

edited

Loading

stevelinton commented Aug 1, 2018

fingolfin commented Aug 1, 2018 •

edited

Loading

fingolfin commented Aug 21, 2018

fingolfin commented Sep 4, 2018

fingolfin commented Sep 4, 2018

fingolfin commented Sep 4, 2018

Reorder methods after new implications are added #2521

Reorder methods after new implications are added #2521

Conversation

stevelinton commented Jun 6, 2018

codecov bot commented Jun 7, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

stevelinton commented Jun 7, 2018

stevelinton commented Jun 8, 2018

stevelinton commented Jun 13, 2018

fingolfin commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevelinton commented Jun 15, 2018

stevelinton commented Jun 28, 2018

fingolfin commented Jul 6, 2018

stevelinton commented Jul 24, 2018

olexandr-konovalov commented Jul 24, 2018 • edited Loading

stevelinton commented Aug 1, 2018

fingolfin commented Aug 1, 2018 • edited Loading

fingolfin commented Aug 21, 2018

fingolfin commented Sep 4, 2018

fingolfin commented Sep 4, 2018

fingolfin commented Sep 4, 2018

codecov bot commented Jun 7, 2018 •

edited

Loading

olexandr-konovalov commented Jul 24, 2018 •

edited

Loading

fingolfin commented Aug 1, 2018 •

edited

Loading