search

peal / vole

A GAP package for backtrack search in permutation groups with graphs
https://peal.github.io/vole
Mozilla Public License 2.0
8 stars 2 forks source link

Vole can return a group on too many points #49

Closed wilfwilson closed 2 years ago

wilfwilson commented 2 years ago 
gap> VoleFind.Group(VoleCon.Stabilise([[]], OnSetsSets) : points := 1);
Group([ (1,2) ])

Probably at some point, the GAP-side validation is taking a minimum of 2 and some other stuff for the number of points to use.

ChrisJefferson commented 2 years ago

Yes, vole in general doesn't like having one point (it gets confused if the partition is already all fixed) so we should probably just error if people ask for points := 1 (or do some special casing -- it has to be the identity group, cosets are harder as you might return fail..)

ChrisJefferson commented 2 years ago

Checking again, points is currently just giving a lower bound, we still generate more points in some cases. We should decide what we want 'points' to do.

ChrisJefferson commented 2 years ago

I actually think on further thought your fix to turn points into "LargestMovedPoint" should fix this bug, and in general produce the right answer. Could we even dump 'points', and just ask people to write VoleCon.LargestMovedPoint?

In general I wouldn't want to make a partition on 3 points when someone is (for example) intersection AlternatingGroup(5) with something, as then you have to decide what to do with "the points not in the partition".

wilfwilson commented 2 years ago

Thanks for thinking about this Chris.

I've already documented the points options as a shorthand for VoleCon.LargestMovedPoint(points), see e.g. https://peal.github.io/vole/doc/chap8_mj.html#X78B2EE748686C51F and I do think that this is the behaviour that we want to implement. I didn't actually realise that we were sometimes generating more points!

I would support getting rid of the points option completely; it is surely irrelevant for canonical image searches, and for the other kinds of searches, we can use VoleCon.LargestMovedPoint(k) (or, as explained at that documentation like, you can just write an integer k and it is interpreted as the constraint VoleCon.LargestMovedPoint(k)).