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:

Status badges

Description (last modified by etn40ff)

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 etn40ff

  • Branch set to u/etn40ff/sub_laurent_poly

comment:2 Changed 5 years ago by etn40ff

  • Authors set to Salvatore Stella
  • 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 git

  • Commit changed from 94635cc51e5ff4c02f13b6c7a65d944c07783374 to cca7f1e6bf7b4a456ba7e527693f48297b9ca9cf

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

cca7f1eFix coercion for LaurentPolynomialRing

comment:4 Changed 5 years ago by etn40ff

  • Branch changed from u/etn40ff/sub_laurent_poly to u/etn40ff/19358
  • Commit changed from cca7f1e6bf7b4a456ba7e527693f48297b9ca9cf to 76ac4339b195762106d406516400f8468de844ef

New commits:

c784bceFix coercion for LaurentPolynomialRing
76ac433Improved doctest

comment:5 Changed 5 years ago by stumpc5

  • Is x.parent().gens() guaranteed to always come in the same order?
  • You use x.parent().gens() and also parent. 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 vdelecroix

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 etn40ff

@stumpc5

  • x.parent().gens() should return x.parent()._gens which is a tuple so the order should be fixed.
  • x.parent().gens() and also parent 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 vdelecroix

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:

  1. if the variables of R are included in the variables of S then the map R -> S is given by names of variables
  2. else if R has less or equal variables than S then the map R -> S is given by the order of the variable
  3. else if some variable of R are present in S then there is a partial map considered
  4. 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 stumpc5

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

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 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?

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 than S then the map R -> S is given by a mapping of the variables of R into S 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 etn40ff

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 jdemeyer

Let me just add that we really should be consistent between laurent polynomials and ordinary polynomials.

comment:14 Changed 5 years ago by etn40ff

  • Description modified (diff)

comment:15 Changed 5 years ago by bruno

Related (I think) tickets: #5225, #16447, #10575.

comment:16 Changed 5 years ago by etn40ff

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 git

  • Commit changed from 76ac4339b195762106d406516400f8468de844ef to a1b0b8d5369562bd122032af825bcdbd82ccb776

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

5b7e7f1Merge branch 'u/etn40ff/19358' of git://trac.sagemath.org/sage into HEAD
a1b0b8dNow only making the minimum changes to fix coercions

comment:18 Changed 5 years ago by etn40ff

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 drupel

  • 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 vbraun

  • Status changed from positive_review to needs_info

The reviewer name should be your real name

comment:21 Changed 5 years ago by drupel

  • Reviewers changed from drupel to Dylan Rupel
  • Status changed from needs_info to positive_review

comment:22 Changed 5 years ago by vbraun

  • Branch changed from u/etn40ff/19358 to a1b0b8d5369562bd122032af825bcdbd82ccb776
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.