Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear stored inverse when calling TrimPerm #3575

Merged
merged 1 commit into from
Jul 17, 2019

Conversation

ChrisJefferson
Copy link
Contributor

@ChrisJefferson ChrisJefferson commented Jul 15, 2019

Fixes #3574 , although this is actually an old bug which was only recently discovered by an interaction between semigroups and a recent change.

This is a minimal fix, so it can be backported (if another release of 4.10 is made). I would like to investigate removing/changing TrimPerm/TRIM_PERM, because it's very easy to abuse and break things, but that's for another PR.

The problem we definitely have: If TrimPerm turns a Perm4 into a Perm2, we need to remove the stored inverse, as a permutation and it's store inverse must have the same TNUM. I think the only place this actually causes a problem is calculating x/p for an integer x and permutation p, after p has been trimmed, but there could be other places.

The reason this PR always throw away the stored inverse: If TrimPerm was used to produce a logically different permutation (say trimming (1,2)(3,4) to just (1,2)), then we also have to remove the inverse. It's unclear to me if that is supposed allowed by TrimPerm. It seems like a terrible idea as permutations are immutable, so I think should be banned.

  • Fix bug in calculating x/p for an integer x and permutation p, if p has been 'trimmed'.

@ChrisJefferson ChrisJefferson added kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes backport-to-4.10 labels Jul 15, 2019
@coveralls
Copy link

coveralls commented Jul 15, 2019

Coverage Status

Coverage increased (+0.0004%) to 84.322% when pulling a8d9607 on ChrisJefferson:fix-inv-perm into 2e9ace1 on gap-system:master.

@wilfwilson
Copy link
Member

Another option (obviously more work) would be, whenever you trim a perm p, to also trim its stored inverse (if it has one) to the same length. Just wondered whether you'd considered this.

@ChrisJefferson
Copy link
Contributor Author

My plan for this PR, given it might be backported, was to do the smallest (hopefully) clearly correct thing. Although I've just realised I failed at that goal, because as well as stopping p pointing at it's inverse, I need to stop it's inverse pointing at p :(

@wilfwilson
Copy link
Member

Whoops! Okay, sure, I'll must make a note on #3576.

@ChrisJefferson
Copy link
Contributor Author

Now with bonus fix!

@codecov
Copy link

codecov bot commented Jul 16, 2019

Codecov Report

Merging #3575 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3575      +/-   ##
==========================================
- Coverage   84.54%   84.51%   -0.03%     
==========================================
  Files         696      695       -1     
  Lines      345127   345202      +75     
==========================================
- Hits       291779   291763      -16     
- Misses      53348    53439      +91
Impacted Files Coverage Δ
src/permutat.h 89.47% <100%> (+5.6%) ⬆️
src/permutat.cc 97.16% <100%> (ø) ⬆️
lib/fieldfin.gi 78.3% <0%> (-10.2%) ⬇️
src/sysfiles.c 39.53% <0%> (-1.87%) ⬇️
src/streams.c 74.92% <0%> (-1.42%) ⬇️
src/stringobj.c 93.51% <0%> (-0.59%) ⬇️
src/listfunc.c 97.33% <0%> (-0.2%) ⬇️
src/hpc/threadapi.c 46.76% <0%> (-0.04%) ⬇️
src/libgap-api.h
lib/error.g 50% <0%> (+10.99%) ⬆️

Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this would also be safe if you put the CLEAR_STOREDINV_PERM(perm); only inside the else if (m <= 65536) { block of code, because that's the only bit that actually changes the type of perm.

But I don't insist, and the performance gain would surely be only marginal - storing the inverse is useful when you use the inverse of a permutation many times; but I imagine a permutation doesn't get trimmed very often, meaning the recalculation of a cleared stored inverse probably doesn't happen very often.

@wilfwilson
Copy link
Member

And unfortunately Travis is failing because https://www.gap-system.org is down 🙁

@wilfwilson
Copy link
Member

(I can't restart the AppVeyor build; maybe you can @ChrisJefferson, otherwise I guess push again to make the tests pass.)

@wilfwilson wilfwilson added the kind: bug Issues describing general bugs, and PRs fixing them label Jul 16, 2019
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I restarted the AppVeyor builds, hopefully that'll fix them.

src/permutat.h Outdated Show resolved Hide resolved
src/permutat.h Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

I think one could change to be parameterless and just compute the "largest moved point" and use that. It'll be a minor cost. Or perhaps it could take the maximum of this and the argument...

That said, I'd greatly prefer if we didn't change permutations in place at all, and got rid of TRIM_PERM completely. However, the orb package uses it for a hash function. But I think we have a "safer" such hashing function in datastructures, so perhaps it could be ported to orb? Then I'd make a release of orb, and once that is in the distro, we could get rid of TRIM_PERM in the master branch.

@fingolfin
Copy link
Member

I took the liberty of updating this PR with the suggestions by Wilf and me.

@ChrisJefferson
Copy link
Contributor Author

I mentioned in the original message why we have to always remove the inverse -- because if we trim (1,2)(3,4) to length 2, then we have a logically different permutation, and therefore a different inverse.

@fingolfin
Copy link
Member

That usage of TRIM_PERM is simply invalid. While it works by chance to call TRIM_PERM( (1,2)(3,4), 2 ), this already creates a broken permutation: TRIM_PERM( (1,2)(3,4), 3 ). We really should just bite the bullet and always compute the largest moved point in TRIM_PERM, IMHO.

I've added a commit which does just that, including test cases.

@ChrisJefferson
Copy link
Contributor Author

The whole reason (I guess) that TRIM_PERM took an argument, instead of just calculating the LargestMovedPoint, is that it was designed to be a very cheap. Actually calculating the LargestMovedPoint makes the function much more expensive.

Also, while I agree TRIM_PERM(p,n) is invalid unless the permutation stabilises [1..n], I wasn't sure if someone might be using it for that purpose (where it would work fine, as long as there were no other references to the permutation).

@fingolfin
Copy link
Member

We can easily enumerate all uses of TRIM_PERM in the library and in packages (only orb is affected), and none of them use this to truncate a permutation (and if they did, that would be a serious bug in that code, IMNSHO). And if we further inspect each existing call to TRIM_PERM, we see that all but one of them actually call LARGEST_MOVED_POINT_PERM or LargestMovedPoint right before. The one exception is TrimStabChain. That function in turn has a single call across the whole library and all package: TrimStabChain(S,LargestMovedPoint(Range(hom)));

This suggests to me that that our best option is to modify TRIM_PERM as I do here, and then modify all callers to avoid their LargestMovedPoint calls if possible. (It is not quite possible for the hash function in lib/dicthf.gi:215 but there it will be called only very rarely, and not anymore after the permutation has been trimmed at least once.

Another variant would be to simply trim permutations as a side effect of LargestMovedPoint, but that might confuse some tests. It shouldn't break any actual code, though -- although that is a somewhat tenuous "shouldn't".

@fingolfin
Copy link
Member

OK, small correction: the call TRIM_PERM( img, LargestMovedPoint( Range( hom ) ) ); in the library actually asks the range group for its largest moved point which may (a) be cached and (b) be actually larger than LargestMovedPoint(img) (whether that is good or bad or irrelevant is another matter). I am somewhat confused as to why ImagesRepresentative call this at all, though -- at least if TrimStabChain was called on the stab chain, it shouldn't be necessary?

This is required if TrimPerm changes the TNUM of the permutation,
as a permutation and its stored inverse must have the same TNUM.
This is also required if TrimPerm changes the permutation (although
this would break many other things if anyone did it)
@fingolfin
Copy link
Member

Anyhow, this requires some extra work, so I've now restored the CLEAR_STOREDINV_PERM() to be the way @ChrisJefferson wants it (though I am still not convinced that it's going to be useful, because in instances where it matters the user already did something wrong; and in all existing calls to TRIM_PERM, it never matters). I also dropped my commit, and will add it to a separate PR, so as to not hold up this important bug fix further!

@fingolfin
Copy link
Member

By the way, once a permutation has been trimmed, LargestMovedPointPerm will be very quick for it, too

@wilfwilson wilfwilson merged commit 1116a0b into gap-system:master Jul 17, 2019
@wucas wucas added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 20, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
@ChrisJefferson ChrisJefferson deleted the fix-inv-perm branch April 1, 2020 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hang at assertion level 2 when computing with a certain group of perms made by PermLeftQuoTransformation
6 participants