Opened 7 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:

Status badges

Description (last modified by vdelecroix)

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 7 years ago by vdelecroix

  • Cc vdelecroix added
  • Description modified (diff)

cc'ing me (+ formatting in the description)

comment:2 Changed 7 years ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Branch set to u/vdelecroix/17692
  • Commit set to e3aa70371159bf5faf231260e8d6e766665f5c04
  • Dependencies set to #17694
  • Status changed from new to needs_review

New commits:

d3e1ad5trac #17694: upgrade the category of polyhedra
c0a0bbbtrac #17694: one_element->one and zero_element->zero
e3aa703trac #17692: rewrite __invert__ (and delete some)

comment:3 Changed 7 years ago by vdelecroix

  • Description modified (diff)

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

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 7 years ago by git

  • Commit changed from e3aa70371159bf5faf231260e8d6e766665f5c04 to 387ff8f217a41cbe64738048cef7bae8cd7167c9

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

387ff8ftrac #17692: remove zero test + override -> overwrite

comment:6 in reply to: ↑ 4 Changed 7 years ago by vdelecroix

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 7 years ago by mmezzarobba

  • 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 7 years ago by git

  • Commit changed from 387ff8f217a41cbe64738048cef7bae8cd7167c9 to 16f36b07f37fae901bdcc8aa29cd98bab1021e82

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

06af245trac #17692: fix interfaces
16f36b0trac #17692: remove a special case (for coercion)

comment:9 Changed 7 years ago by vdelecroix

  • Status changed from needs_work to needs_review

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

  • 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 7 years ago by vdelecroix

Replying to jdemeyer:

Wouldn't it be better to add the zero()/one() functions in the base class src/sage/interfaces/interface.py?

True. Will do.

comment:12 Changed 7 years ago by git

  • Commit changed from 16f36b07f37fae901bdcc8aa29cd98bab1021e82 to 310f284e92526d4b569a7044bc1883cd3190b088

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

310f284trac #17692: move .one and .zero into Interface

comment:13 Changed 7 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:14 Changed 7 years ago by mmezzarobba

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
**********************************************************************
Last edited 7 years ago by mmezzarobba (previous) (diff)

comment:15 Changed 7 years ago by mmezzarobba

  • Status changed from needs_review to needs_work

comment:16 Changed 7 years ago by vdelecroix

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 7 years ago by vdelecroix

  • Dependencies changed from #17694 to #17694, #17740, #17741

comment:18 Changed 6 years ago by jdemeyer

I actually propose to close this as "wontfix" since we need to get rid of __invert__ anyway.

comment:19 Changed 6 years ago by vdelecroix

Why? Where?

comment:20 Changed 6 years ago by jdemeyer

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.

Note: See TracTickets for help on using tickets.