#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 )
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)
Change History (25)
comment:1 Changed 8 years ago by
- Description modified (diff)
comment:2 Changed 8 years ago by
comment:3 Changed 8 years ago by
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.
comment:4 Changed 8 years ago by
- 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
- Milestone changed from sage-5.11 to sage-5.12
comment:6 Changed 7 years ago by
- 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
- Milestone changed from sage-6.1 to sage-6.2
comment:8 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:9 Changed 6 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:10 Changed 5 years ago by
- 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:
8f85af1 | Remove the bogus coercion from SR to QQbar
|
8b0a847 | #14485 regression test
|
comment:11 Changed 5 years ago by
- 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
- 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
- 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
Patchbot: sage -t --long src/sage/graphs/matchpoly.pyx # 1 doctest failed
comment:15 Changed 5 years ago by
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: ↓ 17 Changed 5 years ago by
- 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
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_
asreturn 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 throughZZ
.
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
- 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
- Status changed from needs_work to needs_review
comment:20 follow-up: ↓ 21 Changed 5 years ago by
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: ↓ 24 Changed 5 years ago by
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
- 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
- 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
- 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 hereThe 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.
The weirdness is really just limited to the difference in behaviour between these:
The thing that's failing in the latter case is
and
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 forQQbar['x']
.For instance,
both (rightly) fail,but with significantly different tracebacks. This might give some indication of what is tried for
AA
but not forQQbar
.