{"action":"created","issue":{"url":"https://api.github.com/repos/gap-system/gap/issues/2818","repository_url":"https://api.github.com/repos/gap-system/gap","labels_url":"https://api.github.com/repos/gap-system/gap/issues/2818/labels{/name}","comments_url":"https://api.github.com/repos/gap-system/gap/issues/2818/comments","events_url":"https://api.github.com/repos/gap-system/gap/issues/2818/events","html_url":"https://github.com/gap-system/gap/issues/2818","id":360646248,"node_id":"MDU6SXNzdWUzNjA2NDYyNDg=","number":2818,"title":"Test failures due to method re-ordering","user":{"login":"fingolfin","id":241512,"node_id":"MDQ6VXNlcjI0MTUxMg==","avatar_url":"https://avatars2.githubusercontent.com/u/241512?v=4","gravatar_id":"","url":"https://api.github.com/users/fingolfin","html_url":"https://github.com/fingolfin","followers_url":"https://api.github.com/users/fingolfin/followers","following_url":"https://api.github.com/users/fingolfin/following{/other_user}","gists_url":"https://api.github.com/users/fingolfin/gists{/gist_id}","starred_url":"https://api.github.com/users/fingolfin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fingolfin/subscriptions","organizations_url":"https://api.github.com/users/fingolfin/orgs","repos_url":"https://api.github.com/users/fingolfin/repos","events_url":"https://api.github.com/users/fingolfin/events{/privacy}","received_events_url":"https://api.github.com/users/fingolfin/received_events","type":"User","site_admin":false},"labels":[{"id":180442294,"node_id":"MDU6TGFiZWwxODA0NDIyOTQ=","url":"https://api.github.com/repos/gap-system/gap/labels/kind:%20bug","name":"kind: bug","color":"e11d21","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":12,"created_at":"2018-09-16T14:48:29Z","updated_at":"2018-10-02T23:02:59Z","closed_at":null,"author_association":"MEMBER","body":"[ ping to @alex-konovalov  @stevelinton @frankluebeck @ThomasBreuer @ChrisJefferson @markuspf ]\r\n\r\nHaving merged PR #2773, which updates the ordering of messages as new implications are added (fixing various issues, see issue #2513), we now are seeing a lot of test failures after `LoadAllPackages()`.\r\n\r\nSome of them are fixed by PR #2815, but by far not all. The ones I looked at so far all follow the same pattern: two methods A, B in the GAP library where A used to be ranked above B reverse order when all packages are loaded, due to changes in the relative ranking of their involved filters.\r\n\r\nIn some cases, there already was a static rank adjustment, i.e., by a *fixed* number, applied in the library; but that change becomes insufficient if one filter increases massively when all packages are loaded, the other are not.\r\n\r\nExample 1:\r\n```\r\ngap> Enumerator( Integers mod 9 );\r\n<enumerator of (Integers mod 9)>\r\ngap> LoadAllPackages();\r\n...\r\ngap> Enumerator( Integers mod 9 );\r\n[ ZmodnZObj( 0, 9 ), ZmodnZObj( 1, 9 ), ZmodnZObj( 2, 9 ), ZmodnZObj( 3, 9 ),\r\n  ZmodnZObj( 4, 9 ), ZmodnZObj( 5, 9 ), ZmodnZObj( 6, 9 ), ZmodnZObj( 7, 9 ),\r\n  ZmodnZObj( 8, 9 ) ]\r\n```\r\n\r\nExplanation for example 1: Before loading all packages, this `ViewObj` method is used from `coll.gi:528`:\r\n```gap\r\nInstallMethod( ViewObj,\r\n    \"for an enumerator that perhaps has its own `ViewObj' function\",\r\n    [ IsEnumeratorByFunctions ], 20,\r\n```\r\nNote the manual rank increase by 20. After loading all packages, this method is used instead:\r\n```gap\r\nInstallMethod( ViewObj,\r\n    \"for finite lists\",\r\n    [ IsList and IsFinite ],\r\nfunction( list )\r\n```\r\nYet in a fresh GAP session, started with `gap -A`, we observe this:\r\n```\r\ngap> RankFilter(IsEnumeratorByFunctions);\r\n7\r\ngap> RankFilter(IsList and IsFinite);\r\n4\r\ngap> LoadAllPackages();\r\n...\r\ngap> RankFilter(IsEnumeratorByFunctions);\r\n7\r\ngap> RankFilter(IsList and IsFinite);\r\n139\r\n```\r\n\r\nOops. So the meager static rank adjustment by 20 is not enough. We'd have to use at least 133 instead. But of course that won't work, unless one adds more packages.\r\n\r\nA quick fix for this is the following: since we *always* want to prefer the `IsEnumeratorByFunctions` if it is applicable, we could use this dynamic rank adjustment instead: `{} -> RankFilter(IsList and IsFinite)`. \r\n\r\nThis works, but it leaves me a bit uneasy: for example, the `PrintObj` method for `IsEnumeratorByFunctions` has no rank adjustment, static or dynamic, and it doesn't need one. But it might need one if somebody ever adds a `PrintObj` for, say, `IsList and IsFinite`. I.e., a non-local change could break that code; there is no way to code defensively to prevent this using a simple low static or dynamic rank adjustment.\r\n\r\nSo perhaps a better fix is to use `SUM_FLAGS` here as rank adjustment (strictly speaking, that's still a static rank adjustment, but for practical purposes works fine). After all, we wan these ViewObj and PrintObj methods applied to *all* `IsEnumeratorByFunctions` objects (which, after all, have built-in hooks for setting custom `ViewObj` and `PrintObj` functions). And even if somebody wanted to install a custom method for their `IsEnumeratorByFunctions and IsFOOBAR` objects, they can do it, as long as they also set `SUM_FLAGS` as rank adjustment.\r\n\r\n\r\nA totally different angle to view this problem is to wonder why did the rank of `IsFinite and IsList` increase so much? Most of it comes from `IsFinite`, indeed, `RankFilter(IsFinite) = 136` after all packages are loaded.\r\n\r\nThat's because tons of packages add implications to `IsFinite`, and at least in some cases, I'd argue they are wrong or at least problematic. But there are certainly many legitimate ones...\r\nAnd then of course there are countless *hidden* implications which nevertheless affect the ranking (see issue #2336). So in the end, we can't completely fight this...\r\n\r\nBut in this particular case, the big jump in rank is due to a single implications, set by the `automgrp` package. It does this:\r\n```gap\r\nDeclareProperty(\"IsAmenable\", IsTreeAutomorphismGroup);\r\nInstallTrueMethod(IsAmenable, IsAbelian);\r\nInstallTrueMethod(IsAmenable, IsFinite);\r\n```\r\nSo now `IsFinite` implies `IsAmenable` and that in turn has an hidden implication to `IsTreeAutomorphismGroup`, hence to `IsGroup`. If I change the above lines to\r\n```gap\r\nDeclareProperty(\"IsAmenable\", IsTreeAutomorphismGroup);\r\nInstallTrueMethod(IsAmenable, IsAbelian and IsGroup);\r\nInstallTrueMethod(IsAmenable, IsFinite and IsGroup);\r\n```\r\n.. then I get `RankFilter(IsList and IsFinite) = 6`, which is still less than `RankFilter(IsEnumeratorByFunctions) = 7`, so example 1 is fixed.\r\n\r\nBut of course this is a fragile fix (and not completely under our control, although I'll contact the AutomGrp authors).\r\n\r\nI'll add more examples if I find time, but for now I can already say that we'll have to change something, in the GAP library and in packages:\r\n\r\n1. Switch some methods from static rank adjustments to dynamic (in the GAP library *and* in packages)\r\n2. Add a rank adjustment to some methods that don't have any right now\r\n3. Possibly use `SUM_FLAGS` for some methods; very loosely said: we'll want that if the filter involves a representation (e.g. in example 1, `IsEnumeratorByFunctions` is an and-filter that has `IsEnumeratorByFunctionsRep` amongst it constituents). Perhaps we'd even want to rank representations higher than 1 by default???\r\n4. Perhaps there are other things we should do?"},"comment":{"url":"https://api.github.com/repos/gap-system/gap/issues/comments/426458932","html_url":"https://github.com/gap-system/gap/issues/2818#issuecomment-426458932","issue_url":"https://api.github.com/repos/gap-system/gap/issues/2818","id":426458932,"node_id":"MDEyOklzc3VlQ29tbWVudDQyNjQ1ODkzMg==","user":{"login":"fingolfin","id":241512,"node_id":"MDQ6VXNlcjI0MTUxMg==","avatar_url":"https://avatars2.githubusercontent.com/u/241512?v=4","gravatar_id":"","url":"https://api.github.com/users/fingolfin","html_url":"https://github.com/fingolfin","followers_url":"https://api.github.com/users/fingolfin/followers","following_url":"https://api.github.com/users/fingolfin/following{/other_user}","gists_url":"https://api.github.com/users/fingolfin/gists{/gist_id}","starred_url":"https://api.github.com/users/fingolfin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fingolfin/subscriptions","organizations_url":"https://api.github.com/users/fingolfin/orgs","repos_url":"https://api.github.com/users/fingolfin/repos","events_url":"https://api.github.com/users/fingolfin/events{/privacy}","received_events_url":"https://api.github.com/users/fingolfin/received_events","type":"User","site_admin":false},"created_at":"2018-10-02T23:02:59Z","updated_at":"2018-10-02T23:02:59Z","author_association":"MEMBER","body":"Alll remaining issues seems to be related to semigroups related, so perhaps @wilfwilson and @james-d-mitchell can help. Specifically, after `LoadAllPackages()`, I am seeing:\r\n```\r\n...\r\ntesting: /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/reesmat.tst\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/reesmat.tst:226\r\n# Input is:\r\nIsRegularSemigroup(R);\r\n# Expected output:\r\ntrue\r\n# But found:\r\nError, recursion depth trap (5000)\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/reesmat.tst:898\r\n# Input is:\r\niso := IsomorphismReesMatrixSemigroup(U);;\r\n# Expected output:\r\n# But found:\r\nError, recursion depth trap (5000)\r\n########\r\n     408 ms (239 ms GC) and 46.2MB allocated for reesmat.tst\r\n...\r\ntesting: /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semipperm.tst\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semipperm.tst:35\r\n# Input is:\r\nOne(I);\r\n# Expected output:\r\n<identity partial perm on [ 1, 2, 3 ]>\r\n# But found:\r\nError, Record Element: <rec> must be a record (not a boolean or fail)\r\n########\r\n     462 ms (343 ms GC) and 39.1MB allocated for semipperm.tst\r\ntesting: /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semirel.tst\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semirel.tst:64\r\n# Input is:\r\ngrp := EquivalenceRelationPartition(GreensRRelation(s1));;\r\n# Expected output:\r\n# But found:\r\nError, recursion depth trap (5000)\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semirel.tst:66\r\n# Input is:\r\nSet(List(grp,i->Size(i))) = Set(List(grp1,i->Size(i)));\r\n# Expected output:\r\ntrue\r\n# But found:\r\nError, no method found! For debugging hints type ?Recovery from NoMethodFound\r\nError, no 1st choice method found for `Size' on 1 arguments\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semirel.tst:68\r\n# Input is:\r\nglp := EquivalenceRelationPartition(GreensLRelation(s1));;\r\n# Expected output:\r\n# But found:\r\nError, recursion depth trap (5000)\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semirel.tst:70\r\n# Input is:\r\nSet(List(glp,i->Size(i))) = Set(List(glp1,i->Size(i)));\r\n# Expected output:\r\ntrue\r\n# But found:\r\nError, Variable: 'glp' must have a value\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semirel.tst:72\r\n# Input is:\r\ngjp := EquivalenceRelationPartition(GreensJRelation(s1));;\r\n# Expected output:\r\n# But found:\r\nError, recursion depth trap (5000)\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semirel.tst:74\r\n# Input is:\r\nSet(List(gjp,i->Size(i))) = Set(List(gjp1,i->Size(i)));\r\n# Expected output:\r\ntrue\r\n# But found:\r\nError, Variable: 'gjp' must have a value\r\n########\r\n     456 ms (338 ms GC) and 28.1MB allocated for semirel.tst\r\ntesting: /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semitran.tst\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semitran.tst:242\r\n# Input is:\r\nIsomorphismTransformationSemigroup(I);;\r\n# Expected output:\r\n# But found:\r\nError, no method found! For debugging hints type ?Recovery from NoMethodFound\r\nError, no 1st choice method found for `Enumerate' on 1 arguments\r\nThe 1st argument is 'fail' which might point to an earlier problem\r\n\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semitran.tst:243\r\n# Input is:\r\nBruteForceIsoCheck(last);\r\n# Expected output:\r\ntrue\r\n# But found:\r\nError, no method found! For debugging hints type ?Recovery from NoMethodFound\r\nError, no 1st choice method found for `IsInjective' on 1 arguments\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semitran.tst:245\r\n# Input is:\r\nBruteForceInverseCheck(last2);\r\n# Expected output:\r\ntrue\r\n# But found:\r\nError, no method found! For debugging hints type ?Recovery from NoMethodFound\r\nError, no 1st choice method found for `InverseGeneralMapping' on 1 arguments\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semitran.tst:311\r\n# Input is:\r\nIsomorphismTransformationMonoid(T);;\r\n# Expected output:\r\n# But found:\r\nError, Record Element: <rec> must be a record (not a boolean or fail)\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semitran.tst:312\r\n# Input is:\r\nBruteForceIsoCheck(last);\r\n# Expected output:\r\ntrue\r\n# But found:\r\nError, no method found! For debugging hints type ?Recovery from NoMethodFound\r\nError, no 1st choice method found for `IsInjective' on 1 arguments\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semitran.tst:314\r\n# Input is:\r\nBruteForceInverseCheck(last2);\r\n# Expected output:\r\ntrue\r\n# But found:\r\nError, no method found! For debugging hints type ?Recovery from NoMethodFound\r\nError, no 1st choice method found for `InverseGeneralMapping' on 1 arguments\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semitran.tst:360\r\n# Input is:\r\nBruteForceAntiIsoCheck(last);\r\n# Expected output:\r\ntrue\r\n# But found:\r\nError, no method found! For debugging hints type ?Recovery from NoMethodFound\r\nError, no 1st choice method found for `Enumerate' on 1 arguments\r\nThe 1st argument is 'fail' which might point to an earlier problem\r\n\r\n########\r\n########> Diff in /Users/mhorn/Projekte/GAP/gap.github/tst/testinstall/semitran.tst:362\r\n# Input is:\r\nBruteForceInverseCheck(last2);\r\n# Expected output:\r\ntrue\r\n# But found:\r\nError, no method found! For debugging hints type ?Recovery from NoMethodFound\r\nError, no 1st choice method found for `InverseGeneralMapping' on 1 arguments\r\n########\r\n     480 ms (334 ms GC) and 59.2MB allocated for semitran.tst\r\n```"}}
{"actor":{"display_login":"fingolfin"}}