Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

#14485 closed defect (fixed)

Get rid of the bogus coercion from SR to QQbar

Reported by: tmonteil Owned by: malb
Priority: minor Milestone: sage-7.2
Component: commutative algebra Keywords: QQbar, polynomial
Cc: burcin Merged in:
Authors: Marc Mezzarobba Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 4ba7202 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by mmezzarobba)

Original issue: there is no problem buildind a polynomial over AA:

sage: R = AA[x]                       
sage: P = R(x^6+x^5+x^4-x^3+x^2-x+2/5)

But the same operation does not work on QQbar:

sage: S = QQbar[x]
sage: Q = S(x^6+x^5+x^4-x^3+x^2-x+2/5)
Traceback (most recent call last)
...
NotImplementedError: symbol

Hovewer, this workaround works:

sage: Q = P.change_ring(QQbar)

Moreover the following works :

sage: S.<x> = PolynomialRing(QQbar,'x')
sage: Q = S(x^6+x^5+x^4-x^3+x^2-x+2/5)
sage: S == QQbar[x]
True

Weird isn't it ?

Attachments (1)

trac_14485.patch (673 bytes) - added by nbruin 8 years ago.
Don't pretend there is a coercion from the symbolic ring into QQbar

Download all attachments as: .zip

Change History (25)

comment:1 Changed 8 years ago by tmonteil

  • Description modified (diff)

comment:2 Changed 8 years ago by nbruin

The weirdness is really just limited to the difference in behaviour between these:

sage: AA['x'](var('x'))
x
sage: QQbar['x'](var('x'))
NotImplementedError: symbol

The thing that's failing in the latter case is

QQbar._element_constructor_(x)

and

AA._element_constructor_(x)

fails in exactly the same way. That suggest that in the case of AA['x'] something is tried before resorting to the thing that fails for QQbar['x'].

For instance,

AA['y'](var('x'))
QQbar['y'](var('x'))

both (rightly) fail,but with significantly different tracebacks. This might give some indication of what is tried for AA but not for QQbar.

comment:3 Changed 8 years ago by nbruin

The thing that goes wrong is the following:

sage: S=QQbar['x']
sage: S.base_ring().has_coerce_map_from(SR)
True

Once this is found, it's clear how an element of SR should be converted into QQbar['x']: SR already coerces into QQbar itself, so we should just convert the element into a "constant polynomial".

Except, of course, that SR doesn't really coerce into QQbar: Only "constant" expressions might (and even then, of course pi doesn't), and x of course isn't one of those. So the proper solution is probably to NOT install a coercion from SR to QQbar, because it isn't a coercion anyway. It's only a partial map. Doing so is probably going to be painful for something else.

This problem doesn't arise for AA because it's pretty clear that not even algebraic elements fit in AA, so no coercion is installed.

Changed 8 years ago by nbruin

Don't pretend there is a coercion from the symbolic ring into QQbar

comment:4 Changed 8 years ago by nbruin

  • Priority changed from major to minor

OK, solving a ticket by deleting two lines is just too appealing to pass by. Of course, there is are all kinds of things that fail this way, e.g.

QQbar(1)+sqrt(2)

doesn't work anymore, but QQbar(1)+QQbar(sqrt(2)) of course still does.

comment:5 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:6 Changed 7 years ago by mmezzarobba

  • Cc burcin added

I was going to report another problem caused by the bogus coercion from SR:

sage: CC.has_coerce_map_from(QQbar)
True
sage: QQbar.has_coerce_map_from(SR)
True
sage: CC.has_coerce_map_from(SR)
False

So I'm all in favor of removing it, even if it breaks stuff that should never have worked in the first place.

We might want to put (or actually put back?) a coercion in the other direction, since symbolic expressions can embed elements of QQbar. But SR has coerce maps from RR, CC, RLF, etc., which yield other coercions from QQbar by composition. Can we consider that all these maps are the same because they only differ in the "precision" of the result? Or should all coercions from inexact rings to QQbar be removed? (Note that the coercion from QQ to SR has exactly the same problem.)

comment:7 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:8 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:9 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:10 Changed 5 years ago by mmezzarobba

  • Authors set to Marc Mezzarobba
  • Branch set to u/mmezzarobba/14485-QQbartoSR
  • Commit set to 8b0a847cbf2aa656e6a2fd8c4e0a383df2ae68bc
  • Status changed from new to needs_review

As it turns out, it is possible to remove the coercion from SR to QQbar without breaking too many things. Moreover, most of the workarounds I had to add are there to deal with the case of I and will no longer be necessary once #5355 (or #12715) and/or #18036 are fixed (but I suggest we dont't wait fix this to happen).


New commits:

8f85af1Remove the bogus coercion from SR to QQbar
8b0a847#14485 regression test

comment:11 Changed 5 years ago by git

  • Commit changed from 8b0a847cbf2aa656e6a2fd8c4e0a383df2ae68bc to 9f5b44b3b5e431a92c77af7c52f4984c5c199a49

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

9f5b44b#14485 a few additional tests

comment:12 Changed 5 years ago by jdemeyer

  • Summary changed from Creation of a polynomial over QQbar to Converting a symbolic polynomial to a polynomial over QQbar

comment:13 Changed 5 years ago by mmezzarobba

  • Description modified (diff)
  • Summary changed from Converting a symbolic polynomial to a polynomial over QQbar to Get rid of the bogus coercion from SR to QQbar

comment:14 Changed 5 years ago by rws

Patchbot: sage -t --long src/sage/graphs/matchpoly.pyx # 1 doctest failed

comment:15 Changed 5 years ago by mmezzarobba

Thanks for the notice. It works here, as well as with the other patchbots. I have no idea at all what is going on.

Jeroen, that sage4 is one of your patchbots, isn't it? Would you mind having a quick look at the failing test?

comment:16 follow-up: Changed 5 years ago by vdelecroix

  • Status changed from needs_review to needs_work

You should get rid of the function _late_import since it is not used anymore. And it would be better to simplify the code of _coerce_map_from_ as

return from_par == ZZ or from_par == QQ or from_par == int or \
       from_par == long or from_par == AA or from_par == QQbar

Moreover, I am not sure if coercion from int/long is necessary here since a conversion would exists through ZZ.

comment:17 in reply to: ↑ 16 Changed 5 years ago by mmezzarobba

Replying to vdelecroix:

You should get rid of the function _late_import since it is not used anymore.

Indeed, I didn't notice, thanks!

And it would be better to simplify the code of _coerce_map_from_ as

return from_par == ZZ or from_par == QQ or from_par == int or \
       from_par == long or from_par == AA or from_par == QQbar

The original style made sense to me so I refrained from making unnecessary changes, but if you prefer this version, fine. I also replaced the == with is.

Moreover, I am not sure if coercion from int/long is necessary here since a conversion would exists through ZZ.

AlgebraicNumber_base.__init__ also has a code path for int and long initializers, but they end up being converted to ZZ, so I guess it won't hurt to remove these cases. Done.

comment:18 Changed 5 years ago by git

  • Commit changed from 9f5b44b3b5e431a92c77af7c52f4984c5c199a49 to 4ba720258752fc035549147219161e6568e80cde

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

4ba7202#14485 simplify code according to reviewer's comments

comment:19 Changed 5 years ago by mmezzarobba

  • Status changed from needs_work to needs_review

comment:20 follow-up: Changed 5 years ago by vdelecroix

While working on something else I saw the following comment in symbolic/functions.pyx line 473

# There is no natural coercion from QQbar to the symbolic ring
# in order to support
#     sage: QQbar(sqrt(2)) + sqrt(3)
#     3.146264369941973?
# to work around this limitation, we manually convert
# elements of QQbar to symbolic expressions here     

comment:21 in reply to: ↑ 20 ; follow-up: Changed 5 years ago by mmezzarobba

Replying to vdelecroix:

While working on something else I saw the following comment in symbolic/functions.pyx line 473

# There is no natural coercion from QQbar to the symbolic ring
# in order to support
#     sage: QQbar(sqrt(2)) + sqrt(3)
#     3.146264369941973?
# to work around this limitation, we manually convert
# elements of QQbar to symbolic expressions here     

The patches on this ticket remove this comment and the corresponding code. I think people now agree that even if it was more or less intentional, trying to support this was a bad idea.

comment:22 Changed 5 years ago by vdelecroix

  • Milestone changed from sage-6.4 to sage-7.2
  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

let it be!

comment:23 Changed 5 years ago by vbraun

  • Branch changed from u/mmezzarobba/14485-QQbartoSR to 4ba720258752fc035549147219161e6568e80cde
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:24 in reply to: ↑ 21 Changed 5 years ago by rws

  • Commit 4ba720258752fc035549147219161e6568e80cde deleted

Replying to mmezzarobba:

Replying to vdelecroix:

While working on something else I saw the following comment in symbolic/functions.pyx line 473

# There is no natural coercion from QQbar to the symbolic ring
# in order to support
#     sage: QQbar(sqrt(2)) + sqrt(3)
#     3.146264369941973?
# to work around this limitation, we manually convert
# elements of QQbar to symbolic expressions here     

The patches on this ticket remove this comment and the corresponding code. I think people now agree that even if it was more or less intentional, trying to support this was a bad idea.

See also #17790, #18832, #20312

Note: See TracTickets for help on using tickets.