Opened 3 years ago

Closed 2 years ago

#22573 closed defect (fixed)

Only invert an action if it's invertible

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.6
Component: coercion Keywords: days85
Cc: pelegm Merged in:
Authors: Jeroen Demeyer Reviewers: Julian Rüth, Daniel Krenn
Report Upstream: N/A Work issues:
Branch: e18c726 (Commits) Commit: e18c726d30af85869e56778c1192277858499180
Dependencies: Stopgaps:

Description

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

Change History (19)

comment:1 in reply to: ↑ description ; follow-up: Changed 3 years ago by 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.

comment:2 Changed 3 years ago by pelegm

  • Cc pelegm added

comment:3 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer

comment:4 in reply to: ↑ 1 ; follow-up: Changed 3 years ago by jdemeyer

  • Summary changed from _act_on_() should not return None by default to Check whether _act_on_() returns None

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.

comment:5 Changed 3 years ago by jdemeyer

  • Summary changed from Check whether _act_on_() returns None to Only invert an action if it's invertible

comment:6 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/_act_on____should_not_return_none_by_default

comment:7 follow-up: Changed 3 years ago by jdemeyer

  • Commit set to e18c726d30af85869e56778c1192277858499180
  • Status changed from new to needs_review

New commits:

e18c726Only invert an action if it is invertible

comment:8 Changed 3 years ago by saraedum

  • Reviewers set to Julian Rüth
  • Status changed from needs_review to positive_review

Looks good, assuming that the patchbots are happy.

comment:9 in reply to: ↑ 4 Changed 3 years ago by dkrenn

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.

comment:10 in reply to: ↑ 7 ; follow-ups: Changed 3 years ago by dkrenn

  • Status changed from positive_review to needs_info

Replying to jdemeyer:

New commits:

e18c726Only 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.

comment:11 in reply to: ↑ 10 Changed 3 years ago by dkrenn

Replying to dkrenn:

Replying to jdemeyer:

New commits:

e18c726Only 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.

comment:12 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by 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).

comment:13 Changed 3 years ago by jdemeyer

  • Status changed from needs_info to needs_review

comment:14 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by dkrenn

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.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 3 years ago by 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:

        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()

comment:16 in reply to: ↑ 15 Changed 3 years ago by dkrenn

  • Reviewers changed from Julian Rüth to Julian Rüth, Daniel Krenn

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).

comment:17 Changed 3 years ago by saraedum

  • Keywords days85 added

comment:18 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:19 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/_act_on____should_not_return_none_by_default to e18c726d30af85869e56778c1192277858499180
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.