{"action":"created","issue":{"url":"https://api.github.com/repos/gap-system/gap/issues/2815","repository_url":"https://api.github.com/repos/gap-system/gap","labels_url":"https://api.github.com/repos/gap-system/gap/issues/2815/labels{/name}","comments_url":"https://api.github.com/repos/gap-system/gap/issues/2815/comments","events_url":"https://api.github.com/repos/gap-system/gap/issues/2815/events","html_url":"https://github.com/gap-system/gap/pull/2815","id":360577190,"node_id":"MDExOlB1bGxSZXF1ZXN0MjE1NzkwMjYw","number":2815,"title":"Fix and simplify method reordering for constructors","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},{"id":300489417,"node_id":"MDU6TGFiZWwzMDA0ODk0MTc=","url":"https://api.github.com/repos/gap-system/gap/labels/regression","name":"regression","color":"eb6420","default":false},{"id":689248488,"node_id":"MDU6TGFiZWw2ODkyNDg0ODg=","url":"https://api.github.com/repos/gap-system/gap/labels/release%20notes:%20not%20needed","name":"release notes: not needed","color":"86d7e8","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":7,"created_at":"2018-09-15T21:23:51Z","updated_at":"2018-09-21T08:57:48Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/gap-system/gap/pulls/2815","html_url":"https://github.com/gap-system/gap/pull/2815","diff_url":"https://github.com/gap-system/gap/pull/2815.diff","patch_url":"https://github.com/gap-system/gap/pull/2815.patch"},"body":"For some reason, the method reordering code added in PR #2773 went through methods for constructors in reverse order compared to those for any other kind of operation. There doesn't seem to be a reason for that, and by getting rid of it, we remove a lot of code duplication and simplify the code overall.\r\n\r\n[For reviewing this PR, I recommend activating \"Hide whitespace changes\" in GitHub's \"Diff settings\", which reveals that it really only deletes a block of code and modifies the computation of `rank` in the remaining code -- and then adjusts the indentation of that remaining code.]\r\n\r\nAs a side effect, this fixes a regression, where reordering of constructors\r\nbroke their relative order, because the alternate code for reordering\r\nconstructors contained a bug in the code shifting the methods around. The\r\nresult of that was the following: Without packages, we got this:\r\n\r\n    gap> Perform(MethodsOperation(SymmetricGroupCons,2), function(r) Print(r.rank, \": \", r.info, \"\\n\"); end);\r\n    -59: SymmetricGroupCons: perm group with domain\r\n    -59: SymmetricGroupCons: perm group with degree\r\n    -64: SymmetricGroupCons: pc group with degree\r\n    -66: SymmetricGroupCons: regular perm group with domain\r\n    -66: SymmetricGroupCons: regular perm group with degree\r\n\r\nBut after calling `LoadAllPackages()`, we got this; note how the\r\nmethods are *not* ordered by descending rank anymore:\r\n\r\n    gap> Perform(MethodsOperation(SymmetricGroupCons,2), function(r) Print(r.rank, \": \", r.info, \"\\n\"); end);\r\n    9853: SymmetricGroupCons: for a partition of a ring into residue classes (RCWA)\r\n    -111: SymmetricGroupCons: for an LpGroup and a positive integer\r\n    -117: SymmetricGroupCons: for a partition of a ring into residue classes (RCWA)\r\n    -154: SymmetricGroupCons: regular perm group with domain\r\n    -154: SymmetricGroupCons: regular perm group with degree\r\n    -152: SymmetricGroupCons: pc group with degree\r\n    -150: SymmetricGroupCons: pcp group with degree\r\n    -147: SymmetricGroupCons: perm group with domain\r\n    -147: SymmetricGroupCons: perm group with degree\r\n\r\nThis resulted in `SymmetricGroup(3)` running into an infinite recursion, and\r\nmany other similar problems.\r\n\r\nWith this patch applied, we get the expected result:\r\n\r\n    Perform(MethodsOperation(SymmetricGroupCons,2), function(r) Print(r.rank, \": \", r.info, \"\\n\"); end);\r\n    9853: SymmetricGroupCons: for a partition of a ring into residue classes (RCWA)\r\n    -111: SymmetricGroupCons: for an LpGroup and a positive integer\r\n    -117: SymmetricGroupCons: for a partition of a ring into residue classes (RCWA)\r\n    -147: SymmetricGroupCons: perm group with domain\r\n    -147: SymmetricGroupCons: perm group with degree\r\n    -150: SymmetricGroupCons: pcp group with degree\r\n    -152: SymmetricGroupCons: pc group with degree\r\n    -154: SymmetricGroupCons: regular perm group with domain\r\n    -154: SymmetricGroupCons: regular perm"},"comment":{"url":"https://api.github.com/repos/gap-system/gap/issues/comments/423464350","html_url":"https://github.com/gap-system/gap/pull/2815#issuecomment-423464350","issue_url":"https://api.github.com/repos/gap-system/gap/issues/2815","id":423464350,"node_id":"MDEyOklzc3VlQ29tbWVudDQyMzQ2NDM1MA==","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-09-21T08:57:48Z","updated_at":"2018-09-21T08:57:48Z","author_association":"MEMBER","body":"That's fair. I'll take responsibility then for the changes in here -- and hope that if there are issues with it discovered later on, the author of the original change (@stevelinton) which this PR tries to fix, will help resolve it."}}
{"actor":{"display_login":"fingolfin"}}