Fix is_partial_transformation

[james-d-mitchell]

This is an attempt to fix is_partial_transformation, as far as I can tell the algorithm previously implemented did the following with input epu8 v, size_t k:

1. find the largest value diff where the input v differs from the identity;
2. check if all 16 values are <16 or ==0xFF, if not, return false;
3. return diff == 16 (meaning that v is the identity) or diff < k

I don't think this is at all correct (at least it doesn't correspond to what I expect the function to do, but maybe you had something else in mind @hivert).

Some examples:

• if we are asking is {0, 1, 17, 17, 17, ...} a partial transformation on the first 1 or 2 points, then the answer should be "yes", but 2 above fails.
• if we ask is {0, 1, 255} a partial transformation for any k, then the answer should be "yes", but diff in 1 above is 2, and so if k = 2, then 3 above fails.

I'm not sure if there's a better way of checking than the one I've implemented, in particular, I'm not sure if there's a better way to create MASK.

[hivert]

I'm not sure what I implemented was correct, but the idea was for a transformation on 2 points to lift it to transformation on 16 points by setting that points 2 to 15 are fixed point. I'm not sure for partial transformations but lots of other code for permutations and probably transformations assume that. I don't know if all is properly tested either. If I'm not mistaken, according to this semantic, your counter-examples are not.

[james-d-mitchell]

Thanks for the comments @hivert, I don't completely follow what you've written. Is the argument k supposed to specify that we are restricting the partial transformation to the first k points? If that's true, then the presence or absence of trailing fixed point shouldn't be relevant (or maybe stronger the values of the trailing points shouldn't be relevant).

As I understand it, x = PTransf16({0, 1, 255}) represents the partial identity function missing 2 from its domain and image. If k is used to indicate that we are restricting to the first k points, then I'd expect every restriction of x to be a properly defined partial transformation (the identity on $\{0, 1\}\cup \{3, \ldots, k\}$) with undefined values everywhere else. But according to the original is_partial_transformation function:

    PTransf16 x({0, 1, 255});
    REQUIRE(!x.validate(0));
    REQUIRE(!x.validate(1));
    REQUIRE(!x.validate(2));
    for (size_t i = 3; i <= 16; ++i) {
            REQUIRE(std::pair(i, x.validate(i)) == std::pair(i, true));
    }

equally if x = PTransf16({0, 1, 17, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}), and we are restricting to the first k entries, then I'd expect x.validate(i) to be true for i = 0, 1, 2 and then false for all larger values, but instead with the original is_partial_transformation we get:

   x = PTransf16({0, 1, 2, 17});
   for (size_t i = 0; i <= 16; ++i) {
           REQUIRE(std::pair(i, !x.validate(i)) == std::pair(i, true));
   }

i.e. is_partial_transformation(x, i) is always false. Maybe I misunderstood, but if k does not mean "restrict to the first k entries", then what does it mean? In this example, the values larger than k in x influence the return value of is_partial_transformation, and I can't think of a definition of k where this makes sense.

Another example, if x = PTransf16({255, 1, 2}), this is the partial identity on every point except 0, and the following test passes just fine:

    x = PTransf16({255, 1, 2});
    REQUIRE(!x.validate(0));
    for (size_t i = 1; i <= 16; ++i) {
            REQUIRE(std::pair(i, x.validate(i)) == std::pair(i, true));
    }

This seems to be inconsistent with the first example x = PTransf16({0, 1, 255}).

[hivert]

@james-d-mitchell I answered quickly. Let me try to be more precise. My code is supposed to be a low level representation for partial/permutations/tranformations... To spare memory, to store a transformation of say T_2, I decide not to store the 2 and assume that it will be available from the context. As a consequence I'm in fact only dealing with element of S_16 /T_16 / PT_16. So elements of S_2 /T_2 / PT_2 are dealt by embedding the later into the former. That being said, to have a consistent embedding among these monoids I decided to complete with fixed points. Hence the code I intended to write --I'm not sure if it is right or wrong, an not really in a state where I can consistently check it.

I'm not claiming this is a clever idea, but that was the one I had when I wrote the code. Having decided that, when I designed the other functions, I implicitly made the assumption that all element where actually in S_16 /T_16 / PT_16. So changing this semantic might break some other functions. This need to be checked.

I hope I've made at least my intent clear now.

[james-d-mitchell]

@hivert and I discussed this earlier today and I think I now understand what this function is trying to do, and that my "fix" is not correct, i.e. this is more an issue of documentation rather than a bug in the code. As I understood our discussion is_partial_transformation(x, k) should return true if and only if

1. all 16 values in x are within the correct range (i.e. $[0, 16) \cup \{255\}$); and
2. the values in positions k through 16 are fixed (i.e. they are k through 16).

I'll close this PR when I've got a new one ready that adds this description to the documentation in another PR.