Opened 5 years ago
Closed 5 years ago
#19538 closed defect (fixed)
Fix LaurentPolynomialRing coercion issues
Reported by: | etn40ff | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.10 |
Component: | algebra | Keywords: | |
Cc: | drupel, tscrim | Merged in: | |
Authors: | Salvatore Stella | Reviewers: | Dylan Rupel |
Report Upstream: | N/A | Work issues: | |
Branch: | a1b0b8d (Commits, GitHub, GitLab) | Commit: | a1b0b8d5369562bd122032af825bcdbd82ccb776 |
Dependencies: | Stopgaps: |
Description (last modified by )
LaurentPolynomialRing
has some issues when trying to compare elements from different rings:
sage: L1 = LaurentPolynomialRing(ZZ, 'x0,x1,x2,y0,y1,y2') sage: L2 = LaurentPolynomialRing(ZZ, 'y0,y1,y2') sage: L2.gen(0) in L1 False sage: L1(L2.gen(0)) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) ... TypeError: tuple key must have same length as ngens
The corresponding code for PolynomialRing
yields
sage: P1 = PolynomialRing(ZZ, 'x0,x1,x2,y0,y1,y2') sage: P2 = PolynomialRing(ZZ, 'y0,y1,y2') sage: P2.gen(0) in P1 True sage: P1(P2.gen(0)) y0
This patch should fix them. In the process it also changes this somewhat weird behaviour:
sage: L = LaurentPolynomialRing(ZZ, 'x', 4) sage: R = LaurentPolynomialRing(ZZ, 'x3,x2,t,s') sage: R.inject_variables() Defining x3, x2, t, s sage: L(x3) x0 sage: L(t) x2
NOTE: This behaviour still survives in PolynomialRing
Change History (22)
comment:1 Changed 5 years ago by
- Branch set to u/etn40ff/sub_laurent_poly
comment:2 Changed 5 years ago by
- Cc drupel tscrim added
- Commit set to 94635cc51e5ff4c02f13b6c7a65d944c07783374
- Component changed from PLEASE CHANGE to algebra
- Description modified (diff)
- Status changed from new to needs_review
- Summary changed from Fix LaurentPolynomialRing cohercion issues to Fix LaurentPolynomialRing coercion issues
- Type changed from PLEASE CHANGE to defect
comment:3 Changed 5 years ago by
- Commit changed from 94635cc51e5ff4c02f13b6c7a65d944c07783374 to cca7f1e6bf7b4a456ba7e527693f48297b9ca9cf
comment:4 Changed 5 years ago by
- Branch changed from u/etn40ff/sub_laurent_poly to u/etn40ff/19358
- Commit changed from cca7f1e6bf7b4a456ba7e527693f48297b9ca9cf to 76ac4339b195762106d406516400f8468de844ef
comment:5 Changed 5 years ago by
- Is
x.parent().gens()
guaranteed to always come in the same order? - You use
x.parent().gens()
and alsoparent
. Please use the better of the two always - Have you done some timing tests whether this slows down stuff?
comment:6 Changed 5 years ago by
With respect to the second point (conversion) the behavior should be the same as in PolynomialRing
. Having two different conversion conventions is more confusing than having a "bad" one.
comment:7 Changed 5 years ago by
@stumpc5
x.parent().gens()
should returnx.parent()._gens
which is a tuple so the order should be fixed.
x.parent().gens()
and alsoparent
are in general different, the whole purpose of this patch is to make sure that, when they differ, some natural coercion can still be implemented.
- I did not run precise tests but I expect the new version to be slower than the old one (maybe even significantly slower); the point though is that the current version is not working as it should. I could make a much faster function that returns some hardcoded nonsense but it would clearly not be the right function. More seriously I should run some benchmark, any idea on which?
@vdelecroix
I agree that the behaviour of LaurentPolynomialRing
and that of
PolynomialRing
should be the same. The point is that currently they are
not.
For example the same code in the ticket description evaluates to
sage: L = PolynomialRing(ZZ, 'x0,x1,x2,y0,y1,y2') sage: P = PolynomialRing(ZZ, 'y0,y1,y2') sage: P.inject_variables() Defining y0, y1, y2 sage: y0 in L True sage: L = PolynomialRing(ZZ, 'x', 4) sage: R = PolynomialRing(ZZ, 'x3,x2,t,s') sage: R.inject_variables() Defining x3, x2, t, s sage: L(x3) x0 sage: L(t) x2
I do not know how to fix the first issue in the ticket description while leaving the second one as is. This patch just exchange one inconsistent behavior for another. Any Idea on how to deal with this in a more elegant/consistent/efficient way is extremely welcome.
comment:8 Changed 5 years ago by
Unless I did a mistake, the conversion rules are the same for PolynomialRing
and LaurentPolynomialRing
(but the semantic of the operation in
is not as you noticed). The rules are as follows for a map R -> S
:
- if the variables of
R
are included in the variables ofS
then the mapR -> S
is given by names of variables - else if
R
has less or equal variables thanS
then the mapR -> S
is given by the order of the variable - else if some variable of
R
are present inS
then there is a partial map considered - otherwise an error is raised
sage: R1 = PolynomialRing(ZZ, 'x,y') sage: R2 = PolynomialRing(ZZ, 'x,t') sage: R3 = PolynomialRing(ZZ, 't,x') sage: R4 = PolynomialRing(ZZ, 'x,t,y') sage: map(R4, R1.gens()) # rule 1 [x, y] sage: map(R4, R3.gens()) # rule 1 [t, x] sage: map(R2, R1.gens()) # rule 2 [x, t] sage: map(R1, R2.gens()) # rule 2 [x, y] sage: R1(R4('y')) # rule 3 y sage: R1(R4('t')) # rule 4 Traceback (most recent call last): ... TypeError: Could not find a mapping of the passed element to this ring.
These rules should not change. You can have a look at the implementation of _element_constructor_
for polynomial rings.
comment:9 Changed 5 years ago by
When I do the very same on LaurentPolynomialRing
, I get into trouble, so I don't understand what you mean by "the conversion rules are the same for PolynomialRing
and LaurentPolynomialRing
", see
sage: R1 = LaurentPolynomialRing(ZZ, 'x,y') sage: R2 = LaurentPolynomialRing(ZZ, 'x,t') sage: R3 = LaurentPolynomialRing(ZZ, 't,x') sage: R4 = LaurentPolynomialRing(ZZ, 'x,t,y') sage: map(R4, R1.gens()) # rule 1 --------------------------------------------------------------------------- ... TypeError: tuple key must have same length as ngens sage: map(R4, R3.gens()) # rule 1 --------------------------------------------------------------------------- ... TypeError: tuple key must have same length as ngens sage: map(R1, R2.gens()) # rule 2 [x, y] sage: map(R2, R1.gens()) # rule 2 [x, t] sage: R1(R4('y')) # rule 3 --------------------------------------------------------------------------- ... TypeError: tuple key must have same length as ngens sage: R1(R4('t')) # rule 4 --------------------------------------------------------------------------- ... TypeError: tuple key must have same length as ngens
Is that behaviour what you expected?
comment:10 follow-up: ↓ 11 Changed 5 years ago by
I am not really happy about the formulation of rule 2. What I mean by this is
that I find this example extremely confusing because it maps y
to x
and t
to y
sage: R1 = PolynomialRing(ZZ, 'x,y') sage: R5 = PolynomialRing(ZZ, 'y,t') sage: map(R1, R5.gens()) # rule 2 [x, y]
What is the rationale behind it?
comment:11 in reply to: ↑ 10 Changed 5 years ago by
Replying to etn40ff:
I am not really happy about the formulation of rule 2. What I mean by this is that I find this example extremely confusing because it maps
y
tox
andt
toy
sage: R1 = PolynomialRing(ZZ, 'x,y') sage: R5 = PolynomialRing(ZZ, 'y,t') sage: map(R1, R5.gens()) # rule 2 [x, y]What is the rationale behind it?
I would say: "A map between two rings is always better than a partial map". Whatever you think it is how it works in Sage. If you want to change it you should ask more opinions on sage-devel.
One possibility would be to modify the rule 2 in the following form
2'. else if
R
has less or equal variables thanS
then the mapR -> S
is given by a mapping of the variables ofR
intoS
first trying to match names and then in variable order.
But as I said, the place to discuss it is sage-devel and not a ticket.
comment:12 Changed 5 years ago by
Writing to the mailing list seems the best option here; I will do so shortly. Thanks for the advice
BTW: I think the bug description is describing the wrong issue so I will change it before writing to the list
comment:13 Changed 5 years ago by
Let me just add that we really should be consistent between laurent polynomials and ordinary polynomials.
comment:14 Changed 5 years ago by
- Description modified (diff)
comment:15 Changed 5 years ago by
comment:16 Changed 5 years ago by
Just to ping everyone on this, here is another issue with coercion on LaurentPolynomialRing
sage: R = LaurentPolynomialRing(QQ,'x',2) sage: S = LaurentPolynomialRing(QQ,'x',3) sage: f = S.coerce_map_from(R) sage: f Conversion via _element_constructor_ map: From: Multivariate Laurent Polynomial Ring in x0, x1 over Rational Field To: Multivariate Laurent Polynomial Ring in x0, x1, x2 over Rational Field sage: f(R.gen(0)) Traceback (most recent call last): ... TypeError: tuple key must have same length as ngens
Of course this work as expected on PolynomialRing
comment:17 Changed 5 years ago by
- Commit changed from 76ac4339b195762106d406516400f8468de844ef to a1b0b8d5369562bd122032af825bcdbd82ccb776
comment:18 Changed 5 years ago by
Dear All,
as requested by vdelecroix I posted a question on sage-devel about this issue; Volker's reply was this:
Let me try to summarize the expected behavior: If there is a coercion of the base rings, then there should be a coercion to the (laurent) polynomial ring with additional variables. The variables in the different rings are identified using their name (and not index in gens() or any other rule).Â sage: cm = get_coercion_model() sage: R.<x> = QQ[] sage: S.<x,t> = QQ[] sage: cm.explain(R, S, operator.add) Coercion on left operand via Conversion map: From: Univariate Polynomial Ring in x over Rational Field To: Multivariate Polynomial Ring in x, t over Rational Field Arithmetic performed after coercions. Result lives in Multivariate Polynomial Ring in x, t over Rational Field Multivariate Polynomial Ring in x, t over Rational Field No coercion if variable names are not strict subsets: sage: T.<x,y> = QQ[] sage: cm.explain(T, S, operator.add) Unknown result parent. But explicit conversion can still work, e.g. by falling back to the index in gens(): sage: T(S.gen(0)) x sage: T(S.gen(1)) y
Accordingly I modified this patch so that it only changes the old behaviour when trying to map a laurent polynomial x
into a ring containing all the variables of x.parent()
After the patch the behaviour is
sage: R = LaurentPolynomialRing(QQ,'x2,x0') sage: S = LaurentPolynomialRing(QQ,'x',3) sage: f = S.coerce_map_from(R) sage: f(R.gen(0) + R.gen(1)^2) x0^2 + x2 sage: T = LaurentPolynomialRing(QQ,'x2,x0,t') sage: S.has_coerce_map_from(T) False sage: S(T.gen(0)) x0
Could you please test this?
comment:19 Changed 5 years ago by
- Reviewers set to drupel
- Status changed from needs_review to positive_review
The behavior of LaurentPolynomialRing
coercions and conversions now conforms with the Sage standard as specified by Volker. I am changing to positive review.
comment:20 Changed 5 years ago by
- Status changed from positive_review to needs_info
The reviewer name should be your real name
comment:21 Changed 5 years ago by
- Reviewers changed from drupel to Dylan Rupel
- Status changed from needs_info to positive_review
comment:22 Changed 5 years ago by
- Branch changed from u/etn40ff/19358 to a1b0b8d5369562bd122032af825bcdbd82ccb776
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Fix coercion for LaurentPolynomialRing