Fix sparse pperm #2305

james-d-mitchell · 2018-03-26T07:11:31Z

This PR resolves Issue #2301 by checking that the arguments to PartialPerm are of equal length, if there are more than two of them.

And increases the code coverage of the tests to ~97%, removes unnecessary randomness from the test file, fixes some bugs in the c, and GAP code, and reformats the GAP code.

codecov · 2018-03-26T08:40:00Z

Codecov Report

Merging #2305 into master will increase coverage by 0.24%.
The diff coverage is 99.54%.

@@            Coverage Diff             @@
##           master    #2305      +/-   ##
==========================================
+ Coverage   72.83%   73.07%   +0.24%     
==========================================
  Files         480      480              
  Lines      246475   246622     +147     
==========================================
+ Hits       179508   180213     +705     
+ Misses      66967    66409     -558

Impacted Files	Coverage Δ
lib/pperm.gd	`0% <0%> (ø)`	⬆️
lib/pperm.gi	`100% <100%> (+31.64%)`	⬆️
src/pperm.c	`97.89% <100%> (+11.87%)`	⬆️
src/exprs.c	`93.6% <0%> (+0.17%)`	⬆️

ChrisJefferson · 2018-03-26T08:47:55Z

While we are here (this could go into a different PR),

PartialPerm([1,2],[2^100,2^1000]) also doesn't work, do you also need to check for IsSmallIntRep?

james-d-mitchell · 2018-03-26T08:51:31Z

@ChrisJefferson good idea, I'll adjust this PR for the sake of simplicity.

fingolfin · 2018-03-27T12:57:21Z

@james-d-mitchell do you still plan to make that adjustment? Or should we merge this, and you'll submit another PR later?

james-d-mitchell · 2018-03-27T13:31:15Z

I am making an adjustment, but it will take a little while.

fingolfin · 2018-03-27T13:36:03Z

Great, thanks!

fingolfin

Great job, thanks!

Only thing I am not happy with is the commit message of the first commit: "Resolve issue #2301isn't a great commit title, because it require out-of-band information to be intelligible. better would be e.g. "Fix crash in PartialPerm if length of img and domain differ" -- possibly withFixes #2301` in the body of the commit message.

Other than that, just some minor remarks, which you can address optionally.

fingolfin · 2018-03-29T19:25:48Z

lib/pperm.gi

@@ -225,7 +226,8 @@ InstallMethod(AsPartialPerm, "for a transformation and list",
 [IsTransformation, IsList], 
 function(f, list)

-  if not IsSSortedList(list) or not ForAll(list, IsPosInt) then 
+  if not IsSSortedList(list)
+      or not ForAll(list, x -> IsSmallIntRep(x) and IsPosInt(x)) then 


Off-topic: you can actually write ForAll(list, IsSmallIntRep and IsPosInt); (whether that's a good idea or not is probably a matter of taste; in any case, I am not saying you should change this, just pointing it out ;-)

fingolfin · 2018-03-29T19:29:17Z

src/pperm.c

-        SET_CODEG_PPERM2(join, dej);
+
+    if (def > deg) {
+        return FuncJOIN_IDEM_PPERMS(self, g, f);


Nice way to simplify the following code a lot! As a minor tweak, you could also do this:

SWAP(Obj, f, g); SWAP(UInt, def, deg);

Nice, I didn't know about SWAP, I've updated the PR to use this instead.

fingolfin · 2018-03-29T19:30:52Z

src/pperm.c

@@ -2539,7 +2471,7 @@ Obj FuncShortLexLeqPartialPerm(Obj self, Obj f, Obj g)
        if (DEG_PPERM4(g) == 0)
            return False;
        rankg = RANK_PPERM4(g);
-        domg = DOM_PPERM(f);
+        domg = DOM_PPERM(g);


Oops, good catch!

I couldn't get the code to enter one case, which lead me to spot this 👍

fingolfin · 2018-03-29T19:32:09Z

src/pperm.c

-            else { /* this case cannot occur since I think POW is not defined
-                      */
+            else {
+                // This case cannot occur since POW is not defined


I don't understand this comment. Best I can come up with is that POW is a macro, and clearly defined as such... so clearly you mean something else, but what?

This is an old comment, and I do not know what I meant either, my memory is that I couldn't get the code to enter this case. I will update this.

Actually, I remember what this means now, after putting the analogous code from permutat.c in here instead of the error. I have updated the comment.

fingolfin · 2018-03-29T19:32:51Z

src/pperm.c

@@ -6465,7 +6355,8 @@ Obj FuncOnPosIntSetsPartialPerm(Obj self, Obj set, Obj f)
    }

    PLAIN_LIST(set);
-    res = NEW_PLIST_WITH_MUTABILITY(IS_MUTABLE_PLIST(set), T_PLIST_CYC_SSORT, LEN_LIST(set));
+    res = NEW_PLIST_WITH_MUTABILITY(IS_MUTABLE_PLIST(set), T_PLIST_CYC_SSORT,
+                                    LEN_LIST(set));


Doesn't really matter, but you could use LEN_PLIST here, since we just converted set to a plist in the line before.

Resolve issue gap-system#2301.

This also makes the test much faster.

This change is necessary since none of the kernel methods work for non-small int rep integers.

james-d-mitchell · 2018-04-02T16:53:19Z

@fingolfin I think I have fixed all the things you requested @fingolfin.

fingolfin · 2018-04-02T20:14:38Z

Tests fail in 32bit builds

james-d-mitchell · 2018-04-02T20:30:42Z

Yup, I’ll fix it in the morning

fingolfin · 2018-04-02T20:48:53Z

src/pperm.c

@@ -341,7 +336,7 @@ static Obj PreImagePPermInt(Obj pt, Obj f)
 * GAP functions for partial perms
 *****************************************************************************/

-
+// FIXME why is this a function???
 Obj FuncEmptyPartialPerm(Obj self)


Alas, whether there is a good justification or not, this function is documented in the GAP reference manual, so we cannot just remove it. Of course it is also documented as a "short hand for PartialPerm([]);." So one could replace this C function by a GAP function. And then, if you felt strongly about it, also mark it as obsolete.

Not sure that new C comment is going to help anybody, though ;-).

fingolfin

I am happy with the first commit now, and only have some quibbles about a new comment -- but we can also resolve those post merge shrug.

fingolfin · 2018-04-02T20:51:59Z

src/pperm.c

+                // permutat.c uses the macro POW, which calls PowIntPerm2/4,
+                // which if called with a non-small positive integer returns
+                // that integer, since every permutation fixes every positive
+                // integer (small or not). The methods for PowIntPPerm2/4 give


Huh? So (1,2) fixes 1 ?!? Or perhaps you really mean "since every permutation fixes every large positive integer"?

Oops, will fix.

fingolfin · 2018-04-02T21:11:48Z

src/pperm.c

+                // that integer, since every permutation fixes every positive
+                // integer (small or not). The methods for PowIntPPerm2/4 give
+                // an error if the argument is a non-small integer, and so we
+                // cannot call POW here.


Actually, why does it do that? Why not also have a method for (positive) bigint+pperm which returns the bigint unchanged, mirroring the permutation code?

Yes, we could do that, but for now this is not implemented, and so I opted to fix the comment and make the error message more meaningful.

Also just to point out that partial perms unlike permutations do not fix all points where they are not defined, they are simply undefined on those points, so PowIntPPerm2/4 should return 0 in those cases.

fingolfin · 2018-04-02T21:12:50Z

src/pperm.c

+                // cannot call POW here.
+                ErrorQuit("the first argument (a set) must consist of "
+                          "integers less than 2 ^ %d but contains %s",
+                          NR_SMALL_INT_BITS, (Int)TNAM_OBJ(*ptset));


The standard way in the kernel to phrase such an error would be this: ErrorQuit("<set> must be a list of small integers", 0,0);. This uses that this kernel function is called for OnSets, which is documented as OnSets( set, g ).

But of course the OnSets function for permutation does not contain such a message -- because it relies on POW producing an error, if needed. And that in turn makes it possible for the user resp. packages to install custom methods for \^ -- which then in turn would automatically be useable via OnSets.

I am not saying that's necessarily a good idea, but it's a possibility there. We could of course make it stricter, but then it would be kinda nice if the pperm and perm code would behave similarly for this.

(That said, I'd accept either approach in this PR, we can work on unification in another PR)

I'll update this.

I agree it would be better if the pperm code worked in more or less the same way as the permutat code, but didn't want to make further changes in this PR.

fingolfin · 2018-04-02T21:14:00Z

src/pperm.c

-                ErrorQuit("not yet implemented!", 0L, 0L);
+            else {
+                // This case does not work since PowIntPPerm2/4 only work for
+                // INTOBJ's (i.e. small integers). The analogous code in


We call them "small integers" or sometimes "immediate integers" elsewhere, and I'd stick to that terminology here, w/o brining it up again.

Also: add some further comments, and reformat.

james-d-mitchell · 2018-04-03T08:24:43Z

I've updated this according to your comments @fingolfin and the 32-bit tests should also be fixed by these changes.

james-d-mitchell · 2018-04-05T09:44:20Z

Any reason not to merge this? @fingolfin @ChrisJefferson

fingolfin · 2018-04-05T10:49:19Z

Two reasons why @ChrisJefferson and me don't merge: we are both on vacation; and new changes keep being added to this PR, instead of new PRs, i.e. you added changes right after asking why this isn't being merged... So I won't merge this from here, as I can't review the new changes sensibly from here. But of course somebody else who is not on vacation or strike or busy with other things could step up. Otherwise, I'll get to it next week.

In the meantime, people shall feel free to review and merge all my open PRs, some of which have been open and waiting for weeks or even months.

james-d-mitchell · 2018-04-05T10:53:47Z

Thanks @fingolfin, and apologies for disturbing your vacation, I didn't know. I added two commits today, but otherwise this is unchanged from when you previously approved it.

I'm not sure I have the expertise to review all your open PR's...

james-d-mitchell · 2018-04-05T14:47:20Z

I'll amend the second to last commit to avoid OOB access, and drop the last commit altogether since it's not really related.

This occasionally caused incorrect values to be returned when i == deg and by coincidence ptf2[i] == cpt. When ptf2[deg] is beyond the end of the bag containing f, and so is not valid. For reference, this bug caused errors in: semigroups/Semigroups#296 semigroups/Semigroups#472

james-d-mitchell force-pushed the fix-sparse-pperm branch from 230e236 to 7827d86 Compare March 26, 2018 07:12

james-d-mitchell requested a review from ChrisJefferson March 26, 2018 08:04

james-d-mitchell force-pushed the fix-sparse-pperm branch 2 times, most recently from 354cb5d to 71f5735 Compare March 29, 2018 19:20

james-d-mitchell requested a review from fingolfin March 29, 2018 19:21

fingolfin requested changes Mar 29, 2018

View reviewed changes

james-d-mitchell force-pushed the fix-sparse-pperm branch from 71f5735 to e0ba3cd Compare April 2, 2018 16:50

James Mitchell added 5 commits April 2, 2018 17:51

Fix bug in PartialPerm if length of img and dom differ

32b4ca4

Resolve issue gap-system#2301.

pperm: remove randomness from tests

4bdb4e4

This also makes the test much faster.

pperm: IsPosInt -> IsPosInt and IsSmallIntRep

c60d83c

This change is necessary since none of the kernel methods work for non-small int rep integers.

pperm: add tests and resolve issues in GAP code

66fe347

pperm: add assertions to kernel functions

a510753

james-d-mitchell force-pushed the fix-sparse-pperm branch from e0ba3cd to 431881c Compare April 2, 2018 16:52

fingolfin reviewed Apr 2, 2018

View reviewed changes

fingolfin approved these changes Apr 2, 2018

View reviewed changes

James Mitchell added 3 commits April 3, 2018 09:23

pperm: add tests and resolve issues in C code

fb13e86

Also: add some further comments, and reformat.

pperm: GAP code formatted

4e425fa

pperm: kernel code formatted

37aee87

james-d-mitchell force-pushed the fix-sparse-pperm branch from 431881c to 37aee87 Compare April 3, 2018 08:24

james-d-mitchell mentioned this pull request Apr 5, 2018

bipart: doc and enforce degree limit of 2 ^ 29 - 1 semigroups/Semigroups#472

Merged

james-d-mitchell force-pushed the fix-sparse-pperm branch from 330af54 to 0dbcf48 Compare April 5, 2018 11:15

james-d-mitchell force-pushed the fix-sparse-pperm branch from 0dbcf48 to aedc32e Compare April 6, 2018 09:13

fingolfin approved these changes Apr 6, 2018

View reviewed changes

fingolfin merged commit dd60f28 into gap-system:master Apr 6, 2018

fingolfin mentioned this pull request Apr 22, 2018

Crash in partial perm #2301

Closed

fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) labels May 2, 2018

fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jul 31, 2018

Fix sparse pperm #2305

Fix sparse pperm #2305

Conversation

james-d-mitchell commented Mar 26, 2018 • edited Loading

codecov bot commented Mar 26, 2018 • edited Loading

Codecov Report

ChrisJefferson commented Mar 26, 2018

james-d-mitchell commented Mar 26, 2018

fingolfin commented Mar 27, 2018

james-d-mitchell commented Mar 27, 2018

fingolfin commented Mar 27, 2018

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Mar 29, 2018 • edited Loading

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

james-d-mitchell commented Apr 2, 2018

fingolfin commented Apr 2, 2018

james-d-mitchell commented Apr 2, 2018

Choose a reason for hiding this comment

fingolfin left a 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

james-d-mitchell commented Apr 3, 2018

james-d-mitchell commented Apr 5, 2018 • edited Loading

fingolfin commented Apr 5, 2018

james-d-mitchell commented Apr 5, 2018

james-d-mitchell commented Apr 5, 2018

james-d-mitchell commented Mar 26, 2018 •

edited

Loading

codecov bot commented Mar 26, 2018 •

edited

Loading

fingolfin Mar 29, 2018 •

edited

Loading

james-d-mitchell commented Apr 5, 2018 •

edited

Loading