Opened 2 years ago

Closed 2 years ago

Last modified 22 months ago

#18221 closed defect (fixed)

x / 2 doesn't work for x generator of free Q-algebra

Reported by: darij Owned by:
Priority: major Milestone: sage-6.7
Component: coercion Keywords: coercion, sd67,
Cc: tscrim, SimonKing, nthiery, robertwb, vdelecroix Merged in:
Authors: Travis Scrimshaw Reviewers: Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: 9b14aa5 (Commits) Commit:
Dependencies: Stopgaps:

Description

sage: R.<x> = FreeAlgebra(QQ)
sage: x/2
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-2-749cf4ad628f> in <module>()
----> 1 x/Integer(2)

/home/skraeling/sage/src/sage/structure/element.pyx in sage.structure.element.RingElement.__div__ (build/cythonized/sage/structure/element.c:18393)()
   1955                 return (<RingElement>self)._div_(<RingElement>right)
   1956         global coercion_model
-> 1957         return coercion_model.bin_op(self, right, div)
   1958 
   1959     cpdef RingElement _div_(self, RingElement right):

/home/skraeling/sage/src/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel_cache_maps.bin_op (build/cythonized/sage/structure/coerce.c:7628)()
    849             if xp is yp:
    850                 return op(x,y)
--> 851             action = self.get_action(xp, yp, op, x, y)
    852             if action is not None:
    853                 return (<Action>action)._call_(x, y)

/home/skraeling/sage/src/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel_cache_maps.get_action (build/cythonized/sage/structure/coerce.c:13309)()
   1334             pass
   1335         action = self.discover_action(R, S, op, r, s)
-> 1336         action = self.verify_action(action, R, S, op)
   1337         self._action_maps.set(R, S, op, action)
   1338         return action

/home/skraeling/sage/src/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel_cache_maps.verify_action (build/cythonized/sage/structure/coerce.c:14389)()
   1386 
   1387             if action.left_domain() is not R or action.right_domain() is not S:
-> 1388                 raise RuntimeError, """There is a BUG in the coercion model:
   1389                 Action found for R %s S does not have the correct domains
   1390                 R = %s

RuntimeError: There is a BUG in the coercion model:
                Action found for R <built-in function div> S does not have the correct domains
                R = Free Algebra on 1 generators (x,) over Rational Field
                S = Integer Ring
                (should be Free Algebra on 1 generators (x,) over Rational Field, Rational Field)
                action = Right inverse action by Rational Field on Free Algebra on 1 generators (x,) over Rational Field (<type 'sage.categories.action.InverseAction'>)

Change History (24)

comment:1 Changed 2 years ago by darij

  • Cc tscrim SimonKing nthiery added

comment:2 Changed 2 years ago by SimonKing

┌────────────────────────────────────────────────────────────────────┐
│ Sage Version 6.5.beta4, Release Date: 2014-12-21                   │
│ Type "notebook()" for the browser-based notebook interface.        │
│ Type "help()" for help.                                            │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: R.<x> = FreeAlgebra(QQ)
sage: x/2
1/2*x

What is the problem?

EDIT: I mean, what version are you using?

Last edited 2 years ago by SimonKing (previous) (diff)

comment:3 Changed 2 years ago by darij

I am using 6.7.beta1.

comment:4 Changed 2 years ago by tscrim

This fails is 6.6.rc2 as well.

comment:5 Changed 2 years ago by tscrim

The only changes since 6.6.beta4 in src/structure are in parent from #17981 and in element.pyx:

-include "sage/ext/stdsage.pxi"
 include "sage/ext/python.pxi"
 include "coerce.pxi"
+from sage.ext.stdsage cimport *

I'd be very surprised if the change I made in #17981 broke this.

EDIT - Oh, thats 6.5.beta4...

Last edited 2 years ago by tscrim (previous) (diff)

comment:6 Changed 2 years ago by SimonKing

Note that in the error message it says:

RuntimeError: There is a BUG in the coercion model:
                Action found for R <built-in function div> S does not have the correct domains
                R = Free Algebra on 1 generators (x,) over Rational Field
                S = Integer Ring
                (should be Free Algebra on 1 generators (x,) over Rational Field, Rational Field)

Hence, for a change we do not have the problem of non-unique unique parents or parents that have prematurely been garbage collected, but we have a genuinely wrong domain. The coercion model expects rational field but gets integer ring.

comment:7 Changed 2 years ago by tscrim

Okay, so from 6.5.beta4, the only non-trivial changes to coerce_action.pyx are in using isinstance instead of PY_TYPE_CHECK and adding an __invert__ method to ModuleAction.

comment:8 Changed 2 years ago by tscrim

I'd guess this came from the changes of #17740 from looking at the diff.

comment:9 Changed 2 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • Branch set to public/coercion/fix_actions-18221
  • Commit set to dce923e806cdd021b1b0f5a427870cf76f7bed6e
  • Status changed from new to needs_review

Okay, #17740 is the culprit. What happens is it doesn't add the coercion from ZZ -> QQ which was originally in this line:

action = PrecomposedAction(action, None, K._internal_coerce_map_from(S))

New commits:

dce923eFixing cases when the action should also precompose with a coercion.

comment:10 Changed 2 years ago by tscrim

  • Cc robertwb added
  • Reviewers set to Nicolas Thiéry

comment:11 Changed 2 years ago by git

  • Commit changed from dce923e806cdd021b1b0f5a427870cf76f7bed6e to 6b2e9bbd832fdd672e6b5b36e457926305ce482d

Branch pushed to git repo; I updated commit sha1. New commits:

6b2e9bbAdded test which checks the action map.

comment:12 Changed 2 years ago by tscrim

  • Status changed from needs_review to positive_review

Nicolas looked over the code on my computer and gave it an okay.

comment:13 Changed 2 years ago by nthiery

  • Status changed from positive_review to needs_review

comment:14 Changed 2 years ago by nthiery

Sorry, I misexpressed myself: this looks good to me, but I would not mind having a quick double check by the authors of #17740.

comment:15 Changed 2 years ago by nthiery

  • Cc vdelecroix added

comment:16 Changed 2 years ago by vdelecroix

Hello,

In the doc I would not write Bug :trac:`18221`. It looks like there is a problem, but your are actually checking that there is none. Beyond this trivial comment, I am sorry that we broke that in #17740. And the fix looks like the good one to me.

Vincent

comment:17 Changed 2 years ago by git

  • Commit changed from 6b2e9bbd832fdd672e6b5b36e457926305ce482d to 9b14aa5fcf1684960c93984ef74860d6c26cb1ac

Branch pushed to git repo; I updated commit sha1. New commits:

9b14aa5Change in docstring.

comment:18 Changed 2 years ago by tscrim

I had just copied the message added in #17740, so I changed that docstring too.

comment:19 Changed 2 years ago by darij

Is this pos_rev then?

comment:20 Changed 2 years ago by vdelecroix

ok for me at least

comment:21 Changed 2 years ago by nthiery

  • Status changed from needs_review to positive_review

comment:22 Changed 2 years ago by nthiery

Assuming all tests pass, that's good to go!

comment:23 Changed 2 years ago by vbraun

  • Branch changed from public/coercion/fix_actions-18221 to 9b14aa5fcf1684960c93984ef74860d6c26cb1ac
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:24 Changed 22 months ago by jdemeyer

  • Commit 9b14aa5fcf1684960c93984ef74860d6c26cb1ac deleted
  • Reviewers changed from Nicolas Thiéry to Nicolas M. Thiéry
Note: See TracTickets for help on using tickets.