gap-system / gap

Main development repository for GAP - Groups, Algorithms, Programming, a System for Computational Discrete Algebra
https://www.gap-system.org
GNU General Public License v2.0
814 stars 161 forks source link

no generic `OneSameMutability`? #4161

Open ThomasBreuer opened 4 years ago

ThomasBreuer commented 4 years ago

The file lib/arith.gi contains the following method installations.

InstallOtherMethod( OneSameMutability,
    "for an (immutable) object",
    [ IsObject ],
    function( elm )
    local o;
    if IsMutable( elm ) then
      TryNextMethod();
    fi;
    o:= OneMutable( elm );
    MakeImmutable( o );
    return o;
    end );

InstallOtherMethod( ZeroSameMutability,
    "for an (immutable) object",
    [ IsObject ],
    function(elm)
    local z;
    if IsMutable( elm ) then
      TryNextMethod();
    fi;
    z:= ZeroAttr( elm );
    MakeImmutable( z );
    return z;
    end );

Why does the OneSameMutability method first create a new mutable object, in order to make it immutable? The ZeroSameMutability method directly creates an immutable object. (The MakeImmutable call is unnecessary.)

Background: From the viewpoint of a system that does not distinguish between mutable and immutable objects in the same way as GAP does, a natural point of view for an operation that computes a "one" or "zero" of a GAP object is to return something with the same mutability status. Thus a generic function is tempted to call GAP's OneSameMutability or ZeroSameMutability, respectively. However, this idea does not work as generic as one wants, since OneSameMutability calls OneMutable, for which no method is available for example for domains.

gap> OneSameMutability( GF(2) );
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `OneMutable' on 1 arguments at ...

(Just for curiosity: Is there a reason why ZERO_MUT is a synonym of ZeroMutable, but ONE_MUT is a synonym of OneSameMutability?)

fingolfin commented 4 years ago

First off, I would indeed expect the OneSM and ZeroSM methods to have identical (modulo zero vs. one) semantics and implementations, based on what https://www.gap-system.org/Manuals/doc/ref/chap31.html#X8046262384895B2A says, so I find the difference indeed curious. It likely is a bug, possibly due to some confusion related to what I'll talk about below.

BTW, one thing that worries me about the whole Zero/One system described in the manual section I just linked to is that it's not clear to me in how far methods for these may call the respective other operations; this seems important to avoid accidental infinite recursion. There are some hints as to how certain "defaults" are implemented, but no general rules or restrictions?

Regarding the curious mismatch between ZERO_MUT and ONE_MUT: as you are probably aware, those two names are the names for the respective kernel functions, which are documented as follows

/****************************************************************************
**
*F  ZERO_MUT( <op> )  . . . . . . . . . . . . . . . . . . . zero of an object
**
**  'ZERO_MUT' returns the mutable zero of the object <op>.
*/
...
/****************************************************************************
**
*F  AINV_MUT( <op> )  . . . . . . . . . . . . . additive inverse of an object
**
**  'AINV_MUT' returns the mutable additive inverse of the object <op>.
*/
...
/****************************************************************************
**
*F  ONE_MUT( <op> )    . . . . . . . .  one of an object retaining mutability
**
**  'ONE_MUT' returns the one of the object <op> with the same
**  mutability level as <op>.
*/
...
/****************************************************************************
**
*F  INV_MUT( <op> ) . . . . . . . . inverse of an object retaining mutability
**
**  'INV_MUT' returns the multiplicative inverse of the object <op>.
*/

So for some reason there is an odd mismatch between additive and multiplicative worlds.

I think I run into this confusing mismatch before, and even started to rename things, then gave up as it turned into a bigger rabbit hole than I was willing to go down back then... but perhaps it'd be a good idea to go there now anyway, as this is a potential serious source for bugs. Looking at the git log, I think this may have been an accidental difference: @stevelinton added all these kernel functions back in 2001/2002. I'll quote the commit logs from my git conversion our old, private Mercurial repository:

commit ab9fa6048c4c3ec510b4f549f4515e5cedef55ff
Author: Steve Linton <steve.linton@st-andrews.ac.uk>
Date:   2001-10-26 12:47:25 +0000

    Three changes:

    1. quit from break loop now gets you back to an interactive prompt is
    many more, possibly all, circumstances. Should fix the "UserHasQuit"
    warnings

    2. Zero and Additive inverse now each have three versions:

    mutable              ZeroOp  AdditiveInverseOp   (if a mutable version
                                                         exists)
    same as input        ZERO    AINV
    immutable            Zero    AdditiveInverse

    Most of the time it is correct just to install a method for the
    mutable version.

    3. All I/O (except in a few very specific cases) uses unbuffered read
    and write syscalls, rather than buffered fget* fput* etc. library
    functions.

which added this code to the library, the predecessor of the ZeroSameMutability method you quoted above; but note how it uses ZeroOp (or ZeroMutable as we'd call it today) not ZeroAttr.

InstallOtherMethod( ZEROOp,
        "for an immutable object",
        true,
        [IsObject], 0,
        function(elm)
    local z;
    if IsMutable(elm) then
        TryNextMethod();
    else
        z := ZeroOp(elm);
        MakeImmutable(z);
        return z;
    fi;
end);

Then a few months later, we have this:

commit cee15089f0a705287bfff2dc38d554e5bf684358
Author: Steve Linton <steve.linton@st-andrews.ac.uk>
Date:   2002-01-16 08:51:41 +0000

    Added "Same mutability" version of ONE (kernel macro ONE_MUT, library
    operations ONEOp). Adjusted PowObjInt to use it. replaced kernel
    function  ONE_MATRIX by ONE_MATRIX_IMMUTABLE,
    ONE_MATRIX_SAME_MUTABILITY and ONE_MATRIX_MUTABLE.

which only edited the kernel, and was followed right afterwards by a commit which added this to the library:

InstallOtherMethod( ONEOp,
        "for an immutable object",
        true,
        [IsObject], 0,
        function(elm)
    local o;
    if IsMutable(elm) then
        TryNextMethod();
    else
        o := OneOp(elm);
        MakeImmutable(o);
        return o;
    fi;
end);

which of course is the predecessor to the OneSameMutability method you quote above.

But those commits give no indication why the additive versus multiplication kernel functions have different semantics. Perhaps @stevelinton recalls, but my guess would be that since they were a few months apart, this was a simple accident?

(Another minor tidbit: on the kernel level, all OneMutFuncs are identical to their OneFuncs counterparts (except of course for the one in ariths.c), and likewise for InvMutFuncs vs. InvFuncs.)

Next, this commit replaced Zero, ZeroOp and ZEROOp, One, OneOp, ONEOp, etc. by the names we are still using today:

commit 12205553720ec4151408dc012eb523023c7fd2fe
Author: Steve Linton <steve.linton@st-andrews.ac.uk>
Date:   2002-01-30 16:01:36 +0000

    Rename unary arithmetic operations and associated attributes.   SL

Finally, this commit:

commit 19f83f18389935870a4d072cdf52def8e8df6262
Author: Thomas Breuer <sam@math.rwth-aachen.de>
Date:   2002-02-05 08:05:10 +0000

    adjusted the default methods for unary arithmetic operations to the
    documentation

This then for unknown reasons replaced the call to ZeroMutable by a call to ZeroAttr. So I guess we now have come full circle ;-).