kalmarek / abstractpermutations.jl Goto Github PK

I registered a new version of PermGroups using AbstractPermutations.

First, I did not any more pass my tests since you have defined a method

Base.show(::IO,::MIME"text/plain",::AbstractPerm)

Since I did not define such a method for my Perm, this was hijacking in many cases my own show for Perm. I fixed the problem by defining such a method, but I think you should not define such a method.

I don't pass your tests for the last two reasons below:

• You assumed that a malformed permutation raises an ArgumentError. Since I raised another error type, your test failed and made all the other tests fail (not execute). Since I don't mind, I made my functions return ArgumentError but I think you should fix your tests so that at least they continue even if another error type is returned.
• I introduced a checking constructor, but, as I always do, I made check a keyword argument and not an optional argument. I will not change this since I think it is much clearer what it means when you encounter in some code Perm(v;check=false) versus Perm(v,false).
• In your tests the constructor is called in the form Perm{T}(v). I did not define such a constructor, since it is not necessary. If you want to convert the argument you may always do Perm(T.(v)). (I have a constructor Perm{T}(i1,...,in) for a cycle but that is a different circumstance).

It seems to me that your AbstractPermutations is not a minimal compatibility package, but a much more extended one representing your idea of how to write permutations.

By the way, I reiterate my opposition to using the term degree for the largest moved point. Exporting this name means in particular that you cannot load simultaneously this package and any package implementing polynomials.

My permutations would not pass your tests, and here are some details.

If I were to use degree for largest_moved_point, I would fail the test where you define the degree of the identity permutation to be 1. Why not 0? As a mathematician, I would have expected you to make a more consistent choice. The fact that for you cycles(identity)=[[1]] is perhaps a consequence but is inconsistent with the printing of identity as ().

I suppose your firstmoved is my smallest_moved_point. Again it should be 0 and not 1 for the identity. The fixedpoints and nfixedpoints seem to operate on 1:degree. This not very useful if you consider permutations in a permutation group of degree n. This is a case where the "implementation detail" of what is the actual length of the internal image vector would be a more pertinent parameter.

My Perms do not pass the test where the constructor taking a Vector{Int} checks its argument for validity. For me this constructor is almost exclusively used when programming some code internal to the permutations package, and does not check in order to be fast. The most common constructor for the user is using the cycle decomposition like Perm(1,2,3)*Perm(4,5) or perm"(1,2,3)(4,5)". It is these last constructors which are checked.

Another divergence is that for you action like 1^p preserves the type of the constant 1. Rather, in my implementation the result is always of what you call the inttype of the permutation. This is both for having this operation as fast as possible and for consistency. For example, the cycles of a Perm{UInt8} are Vector{UInt8}.

The sign function is usual, but not your parity function which could be
omitted. Similarly, for conjugating permutations, the notation a^bfor inv(b)ab is standard, but not overloading for the same purpose the function conj, which could be omitted.

You check that permutations can be hashed, which is indeed useful in order to be able to use them as keys in Dicts. I claim that they should also have an isless method, in order to be sortable (many algorithms are faster using sorting rather than hashing). Lexicographical order of image vectors does fine for isless.

I have much more to say but that's enough for now.

This issue is used to trigger TagBot; feel free to unsubscribe.

If you haven't already, you should update your TagBot.yml to include issue comment triggers.
Please see this post on Discourse for instructions and more details.

If you'd like for me to do this for you, comment TagBot fix on this issue.
I'll open a PR within a few hours, please be patient!

• isless - the lexicographical ordering #18
• other orders (e.g. deglex via defining DegLex <: Base.Order.Ordering + lt(DegLex(), p, q) #18
• Base.:(^)(p::Permutation, i::Integer). (e.g. for small (in absvalue) is use i^p^p^p^p for larger compute cycle decomposition and shift). #17