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

ListPerm doesn't check its arguments, can give nonsense #4205

Closed
wilfwilson opened this issue Jan 5, 2021 · 5 comments · Fixed by #5566
Closed

ListPerm doesn't check its arguments, can give nonsense #4205

wilfwilson opened this issue Jan 5, 2021 · 5 comments · Fixed by #5566
Labels
kind: quirk Issues that are not bugs, but a discrepancy between user expectation and system behavior topic: error handling topic: library

Comments

@wilfwilson
Copy link
Member

wilfwilson commented Jan 5, 2021

ListPerm is a global function that doesn't validate its arguments.

When given input that is valid (according to its documentation), it behaves fine. Unfortunately people don't always give valid input, such as me 10 minutes ago, when I forgot the brackets in a permutation and wrote ListPerm(5,20).

gap> ListPerm(5,20);
[ 1, 32, 243, 1024, 3125, 7776, 16807, 32768, 59049, 100000, 161051, 248832,
  371293, 537824, 759375, 1048576, 1419857, 1889568, 2476099, 3200000 ]
gap> last = List([1..20],i->i^5);
true
gap> last2 = OnTuples([1 .. 20], 5);
true
gap> ListPerm(1,10);
[ 1 .. 10 ]
gap> ListPerm( One(GL(3,5)), 20 );
[ 1 .. 20 ]

I suggest ListPerm should give an error if its first argument isn't a perm and its optional second argument isn't a positive (or non-negative?) integer .

@wilfwilson wilfwilson added topic: library topic: error handling kind: quirk Issues that are not bugs, but a discrepancy between user expectation and system behavior labels Jan 5, 2021
@fingolfin
Copy link
Member

I agree that these checks should be made.

In addition, perhaps we should also change OnPoints, OnPairs, OnTuples and OnSets to validate their input more. Because that's is the underlying cause for what you observe; essentially, OnTuples(T, g) is equivalent to List(T, x -> x^g):

gap> OnTuples([1..20], 5);
[ 1, 32, 243, 1024, 3125, 7776, 16807, 32768, 59049, 100000, 161051, 248832, 371293, 537824, 759375, 1048576,
  1419857, 1889568, 2476099, 3200000 ]
gap> OnSets([1..20], 5);
[ 1, 32, 243, 1024, 3125, 7776, 16807, 32768, 59049, 100000, 161051, 248832, 371293, 537824, 759375, 1048576,
  1419857, 1889568, 2476099, 3200000 ]

These kernel functions could reject the case where g is not either a perm / transformation / partial perm, or else an external object.

On the other hand, no serious harm is done by the current behavior?

@frankluebeck
Copy link
Member

These function can be performance critical in some code. Before adding tests one should check thoroughly that the performance loss is negligible for all kinds of valid arguments.

@fingolfin
Copy link
Member

If you mean OnSets etc: The test would be a simple TNUM test in the kernel in the code for non-permutations. I strongly doubt that would have a measurable overhead.

For ListPerm: if performance critical code relies on that, it probably should be rewritten...

@wilfwilson
Copy link
Member Author

perhaps we should also change OnPoints, OnPairs, OnTuples and OnSets to validate their input more.
...
On the other hand, no serious harm is done by the current behavior?

I don't see any harm in the current behaviour of OnTuples etc, and moreover this behaviour is meaningful, and therefore potentially useful. Perhaps the documentation for these functions should be generalised to point out that they also work outside of the context of group actions.

@fingolfin
Copy link
Member

I am fine either way, but I do think that ListPerm should raise an error if the arguments have nonsense type.

@wilfwilson wilfwilson changed the title ListPerm doesn't check its arguments, can give nonsense ListPerm doesn't check its arguments, can give nonsense Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: quirk Issues that are not bugs, but a discrepancy between user expectation and system behavior topic: error handling topic: library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants