Opened 8 years ago
Last modified 6 years ago
#17692 needs_work enhancement
The default __invert__ method of an element should avoid coercion
Reported by: | Stefan | Owned by: | |
---|---|---|---|
Priority: | trivial | Milestone: | sage-6.5 |
Component: | basic arithmetic | Keywords: | |
Cc: | vdelecroix | Merged in: | |
Authors: | Vincent Delecroix | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | u/vdelecroix/17692 (Commits, GitHub, GitLab) | Commit: | 310f284e92526d4b569a7044bc1883cd3190b088 |
Dependencies: | #17694, #17740, #17741 | Stopgaps: |
Description (last modified by )
Currently, the __invert__
methods in element.pyx do
return 1/self
This results in a coercion of the integer 1 into the appropriate parent structure. Better would be to do
return self.parent().one() / self
Additionally, the __invert__
method in MultiplicativeGroupElement
relies on a method is_one()
that is not provided by the class itself.
Change History (20)
comment:1 Changed 8 years ago by
- Cc vdelecroix added
- Description modified (diff)
comment:2 Changed 8 years ago by
- Branch set to u/vdelecroix/17692
- Commit set to e3aa70371159bf5faf231260e8d6e766665f5c04
- Dependencies set to #17694
- Status changed from new to needs_review
comment:3 Changed 8 years ago by
- Description modified (diff)
comment:4 follow-up: ↓ 6 Changed 8 years ago by
Why do you add
if self.is_zero(): raise ZeroDivisionError("division by zero in a ring")
I would say it's the division code which should raise this error, not __invert__
.
comment:5 Changed 8 years ago by
- Commit changed from e3aa70371159bf5faf231260e8d6e766665f5c04 to 387ff8f217a41cbe64738048cef7bae8cd7167c9
Branch pushed to git repo; I updated commit sha1. New commits:
387ff8f | trac #17692: remove zero test + override -> overwrite
|
comment:6 in reply to: ↑ 4 Changed 8 years ago by
Replying to jdemeyer:
Why do you add
...
I would say it's the division code which should raise this error, not
__invert__
.
Right. Removed in the last commit.
comment:7 Changed 8 years ago by
- Status changed from needs_review to needs_work
********************************************************************** File "src/sage/structure/coerce.pyx", line 463, in sage.structure.coerce.CoercionModel_cache_maps.explain Failed example: cm.explain(ZZ['x'], QQ['x'], operator.div) Expected: Coercion on left operand via Ring morphism: From: Univariate Polynomial Ring in x over Integer Ring To: Univariate Polynomial Ring in x over Rational Field Defn: Induced from base ring by Natural morphism: From: Integer Ring To: Rational Field Arithmetic performed after coercions. Result lives in Fraction Field of Univariate Polynomial Ring in x over Rational Field Fraction Field of Univariate Polynomial Ring in x over Rational Field Got: Coercion on left operand via Ring morphism: From: Univariate Polynomial Ring in x over Integer Ring To: Univariate Polynomial Ring in x over Rational Field Defn: Induced from base ring by Natural morphism: From: Integer Ring To: Rational Field Arithmetic performed after coercions. Result lives in Univariate Polynomial Ring in x over Rational Field Univariate Polynomial Ring in x over Rational Field ********************************************************************** File "src/sage/structure/coerce.pyx", line 684, in sage.structure.coerce.CoercionModel_cache_maps.division_parent Failed example: cm.division_parent(ZZ['x']) Expected: Fraction Field of Univariate Polynomial Ring in x over Integer Ring Got: Univariate Polynomial Ring in x over Integer Ring **********************************************************************
********************************************************************** File "src/sage/interfaces/maxima_abstract.py", line 2299, in sage.interfaces.maxima_abstract.MaximaAbstractElementFunction.__inv__ Failed example: ~f Expected: 1/sin(x) Got: one()/sin(x) **********************************************************************
********************************************************************** File "src/sage/interfaces/r.py", line 22, in sage.interfaces.r Failed example: ~x Expected: [1] 0.09615385 0.17857143 0.32258065 0.15625000 0.04608295 Got: [1] 0 0 0 0 0 **********************************************************************
comment:8 Changed 8 years ago by
- Commit changed from 387ff8f217a41cbe64738048cef7bae8cd7167c9 to 16f36b07f37fae901bdcc8aa29cd98bab1021e82
comment:9 Changed 8 years ago by
- Status changed from needs_work to needs_review
comment:10 follow-up: ↓ 11 Changed 8 years ago by
- Status changed from needs_review to needs_work
Wouldn't it be better to add the zero()
/one()
functions in the base class src/sage/interfaces/interface.py
?
comment:11 in reply to: ↑ 10 Changed 8 years ago by
Replying to jdemeyer:
Wouldn't it be better to add the
zero()
/one()
functions in the base classsrc/sage/interfaces/interface.py
?
True. Will do.
comment:12 Changed 8 years ago by
- Commit changed from 16f36b07f37fae901bdcc8aa29cd98bab1021e82 to 310f284e92526d4b569a7044bc1883cd3190b088
Branch pushed to git repo; I updated commit sha1. New commits:
310f284 | trac #17692: move .one and .zero into Interface
|
comment:13 Changed 8 years ago by
- Status changed from needs_work to needs_review
comment:14 Changed 8 years ago by
More doctest failures.
********************************************************************** File "src/sage/rings/infinity.py", line 93, in sage.rings.infinity Failed example: unsigned_oo/0 Expected: Traceback (most recent call last): ... ValueError: unsigned oo times smaller number not defined Got: ... File "sage/structure/coerce.pyx", line 798, in sage.structure.coerce.CoercionModel_cache_maps.bin_op (build/cythonized/sage/structure/coerce.c:7153) return PyObject_CallObject(op, xy) File "sage/structure/element.pyx", line 2013, in sage.structure.element.RingElement.__div__ (build/cythonized/sage/structure/element.c:18317) return (<RingElement>self)._div_(<RingElement>right) File "sage/structure/element.pyx", line 2017, in sage.structure.element.RingElement._div_ (build/cythonized/sage/structure/element.c:18420) cpdef RingElement _div_(self, RingElement right): File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/rings/infinity.py", line 458, in _div_ return self * ~other File "sage/structure/element.pyx", line 2073, in sage.structure.element.RingElement.__invert__ (build/cythonized/sage/structure/element.c:19009) return self._parent.one() / self File "sage/structure/element.pyx", line 2013, in sage.structure.element.RingElement.__div__ (build/cythonized/sage/structure/element.c:18317) return (<RingElement>self)._div_(<RingElement>right) File "sage/structure/element.pyx", line 2017, in sage.structure.element.RingElement._div_ (build/cythonized/sage/structure/element.c:18420) cpdef RingElement _div_(self, RingElement right): File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/rings/infinity.py", line 814, in _div_ raise ValueError("quotient of number < oo by number < oo not defined") ValueError: quotient of number < oo by number < oo not defined **********************************************************************
sage -t --warn-long 30.1 src/sage/schemes/elliptic_curves/formal_group.py ********************************************************************** File "src/sage/schemes/elliptic_curves/formal_group.py", line 431, in sage.schemes.elliptic_curves.formal_group.EllipticCurveFormalGroup.inverse Failed example: F = E.formal_group().group_law(6) Exception raised: ... TypeError: unsupported operand parent(s) for '*': 'Multivariate Polynomial Ring in a1, a2, a3, a4, a6 over Rational Field' and 'Multivariate Polynomial Ring in t1, t2 over Multivariate Polynomial Ring in a1, a2, a3, a4, a6 over Integer Ring' **********************************************************************
********************************************************************** File "src/sage/schemes/elliptic_curves/formal_group.py", line 523, in sage.schemes.elliptic_curves.formal_group.EllipticCurveFormalGroup.group_law Failed example: F(t1, 0) Expected: t1 + O(t1, t2)^5 Got: t1 + O(t1, t2)^7 **********************************************************************
comment:15 Changed 8 years ago by
- Status changed from needs_review to needs_work
comment:16 Changed 8 years ago by
Hello,
Indeed! The coercion with polynomial is to blame here... see this sage-devel thread. Several people fixed some issue locally (e.g. #7711). In order to go further, I need the coercion to be fixed. I will open a subsequent ticket if it is not yet here.
Vincent
comment:17 Changed 8 years ago by
- Dependencies changed from #17694 to #17694, #17740, #17741
comment:18 Changed 6 years ago by
I actually propose to close this as "wontfix" since we need to get rid of __invert__
anyway.
comment:19 Changed 6 years ago by
Why? Where?
comment:20 Changed 6 years ago by
Well, everywhere really because __invert__
is supposed to return the bitwise inverse, not the mathematical inverse. It's the whole reason why I came up with the idea of a unary division.
cc'ing me (+ formatting in the description)