Only invert an action if it's invertible

[jdemeyer]

This should be an error:

sage: [1,2,3]/2

I traced this to

sage: (1/2)._act_on_([1,2,3], False)

which uses the default implementation of _act_on_ from src/sage/structure/element.pyx:

    cpdef _act_on_(self, x, bint self_on_left):
        """
        Use this method to implement ``self`` acting on ``x``.

        Return None or raise a CoercionException if no
        such action is defined here.
        """
        return None

CC: @pelegm

Component: coercion

Keywords: days85

Author: Jeroen Demeyer

Branch/Commit: e18c726

Reviewer: Julian Rüth, Daniel Krenn

Issue created by migration from https://trac.sagemath.org/ticket/22573

[dkrenn]

Replying to @jdemeyer:

I traced this to

sage: (1/2)._act_on_([1,2,3], False)

which uses the default implementation of _act_on_ from src/sage/structure/element.pyx:

    cpdef _act_on_(self, x, bint self_on_left):
        """
        Use this method to implement ``self`` acting on ``x``.

        Return None or raise a CoercionException if no
        such action is defined here.
        """
        return None

When I see this correctly, this is does not cause the problem.

There is an action of an integer (from ZZ) on an list as in pure Python (repeating the list). But clearly there is no such action of QQ.
discover_action in sage.structure.coerce discovers an action in the following way: We have a list divided by ZZ-integer; the code says we do as with mul, but then precompose the action by the natural embedding of ZZ in QQ and use an inverse action.

I am not sure what the correct solution would be. In some sense, the correct discovered action should be QQ on list and precompose it with invert element first.

[jdemeyer]

Author: Jeroen Demeyer

[jdemeyer]

comment:4

Replying to @dkrenn:

Replying to @jdemeyer:

I traced this to

sage: (1/2)._act_on_([1,2,3], False)

which uses the default implementation of _act_on_ from src/sage/structure/element.pyx:

    cpdef _act_on_(self, x, bint self_on_left):
        """
        Use this method to implement ``self`` acting on ``x``.

        Return None or raise a CoercionException if no
        such action is defined here.
        """
        return None

When I see this correctly, this is does not cause the problem.

Yes, maybe you are right. The problem is that not all places calling _act_on_ check for None. I still think that the return None convention is fragile, but it's used in many places. So maybe you should keep that.

[jdemeyer]

Branch: u/jdemeyer/_act_on____should_not_return_none_by_default

[jdemeyer]

New commits:

e18c726

Only invert an action if it is invertible

[jdemeyer]

Commit: e18c726

[saraedum]

Reviewer: Julian Rüth

[saraedum]

comment:8

Looks good, assuming that the patchbots are happy.

[dkrenn]

comment:9

Replying to @jdemeyer:

Replying to @dkrenn:

Replying to @jdemeyer:

I traced this to

sage: (1/2)._act_on_([1,2,3], False)

which uses the default implementation of _act_on_ from src/sage/structure/element.pyx:

    cpdef _act_on_(self, x, bint self_on_left):
        """
        Use this method to implement ``self`` acting on ``x``.

        Return None or raise a CoercionException if no
        such action is defined here.
        """
        return None

When I see this correctly, this is does not cause the problem.

Yes, maybe you are right. The problem is that not all places calling _act_on_ check for None. I still think that the return None convention is fragile, but it's used in many places. So maybe you should keep that.

I think it is quite safe to replace the return None in all _act_on_ (there are only 11 occurrences) by a NotImplementedError or similar. Usually they should not be called as the coercion model determines in a different way if there is an action; it is called only when computing the actual value. Thus, if "accidentally" called, at least the error appears.

[dkrenn]

comment:10

Replying to @jdemeyer:

New commits:

e18c726

Only invert an action if it is invertible

-            else:
-                K = G._pseudo_fraction_field()
-                Action.__init__(self, K, action.underlying_set(), action._is_left)
-                self._action = action
-                return

I am not convinced that this is a good solution. Overall, the coercion model correctly determines the way to deal with the situation in general, but in our particular example, there simply is no such action implemented. Deleting the code above prevents finding such an action at all, which is also not good.

[dkrenn]

comment:11

Replying to @dkrenn:

Replying to @jdemeyer:

New commits:

e18c726	Only invert an action if it is invertible

-            else:
-                K = G._pseudo_fraction_field()
-                Action.__init__(self, K, action.underlying_set(), action._is_left)
-                self._action = action
-                return

I am not convinced that this is a good solution. Overall, the coercion model correctly determines the way to deal with the situation in general, but in our particular example, there simply is no such action implemented. Deleting the code above prevents finding such an action at all, which is also not good.

Maybe something along the lines of

cm = sage.structure.element.get_coercion_model()
action = cm.get_action(K, action.underlying_set(), operator.mul)
if action is not None:
    self._action = action
    ...

could do it, but I am not sure, if it is intended to call get_action, etc. at this level (in the action-class itself). And I am very doubtful about operator.mul as at the level of the action-class, everything is somehow independent of the operator.

[jdemeyer]

comment:12

Replying to @dkrenn:

Overall, the coercion model correctly determines the way to deal with the situation in general

I don't agree with this. The code is assuming that, if there is an action of ZZ on X, there is also an action of QQ on X. There is no reason at all in general why that should be the case. Instead, it should start from an action of QQ on X and then invert that (which is what the already-existing code in the coercion model does).

[dkrenn]

comment:14

Replying to @jdemeyer:

Replying to @dkrenn:

Overall, the coercion model correctly determines the way to deal with the situation in general

I don't agree with this. The code is assuming that, if there is an action of ZZ on X, there is also an action of QQ on X. There is no reason at all in general why that should be the case. Instead, it should start from an action of QQ on X and then invert that (which is what the already-existing code in the coercion model does).

This was formulated too unprecise of me, sorry. What I meant is, that the coercion model is at the moment able to discover an action, if it exists (but maybe discovers one that does not exist as no implentation or math-sense). By your changes, you eliminate the discovering of this situation at all (also if there would be an action of QQ (or K). Thus I suggest to search for the action at this point more carefully; I am just not sure how to do this correctly.

[jdemeyer]

comment:15

Replying to @dkrenn:

By your changes, you eliminate the discovering of this situation at all

Of course I do, because it is the right thing to do.

If I want an action of QQ on X, there is no point in looking for an action of ZZ on X. Because even if an action of ZZ on X exists, it does not imply that there is an action of QQ on X.

Note that the code in the coercion model already looks for an action from the fraction field:

        if op is div:
            # Division on right is the same acting on right by inverse, if it is so defined.
            right_mul = None
            try:
                right_mul = self.get_action(R, S, mul)
            except NotImplementedError:
                self._record_exception()

            if right_mul is not None and not right_mul.is_left():
                try:
                    action = ~right_mul
                    if action.right_domain() != S:
                        action = PrecomposedAction(action, None,
                                                   action.right_domain()._internal_coerce_map_from(S))
                    return action
                except TypeError: # action may not be invertible
                    self._record_exception()

            # It's possible an action is defined on the fraction field itself.
            try:
                K = S._pseudo_fraction_field()
            except AttributeError:
                pass
            else:
                if K is not S:
                    try:
                        right_mul = self.get_action(R, K, mul)
                    except NotImplementedError:
                        self._record_exception()

                    if right_mul is not None and not right_mul.is_left():
                        try:
                            return PrecomposedAction(~right_mul, None, K.coerce_map_from(S))
                        except TypeError: # action may not be invertible
                            self._record_exception()

[dkrenn]

Changed reviewer from Julian Rüth to Julian Rüth, Daniel Krenn

[dkrenn]

comment:16

Replying to @jdemeyer:

Replying to @dkrenn:

By your changes, you eliminate the discovering of this situation at all

Of course I do, because it is the right thing to do.

If I want an action of QQ on X, there is no point in looking for an action of ZZ on X. Because even if an action of ZZ on X exists, it does not imply that there is an action of QQ on X.

Note that the code in the coercion model already looks for an action from the fraction field: [...]

Ok, I see. Sorry for the noise. Patch is good then. (modulo patchbot; I'd be happy to put it (back) to positive review once the patchbot agrees).

[saraedum]

Changed keywords from none to days85

[vbraun]

Changed branch from u/jdemeyer/_act_on____should_not_return_none_by_default to e18c726