Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change_ring broken on polynomials #25022

Closed
videlec opened this issue Mar 22, 2018 · 12 comments
Closed

change_ring broken on polynomials #25022

videlec opened this issue Mar 22, 2018 · 12 comments

Comments

@videlec
Copy link
Contributor

videlec commented Mar 22, 2018

If the polynomial can be coerced in the new base ring, the result of change_ring is a constant

sage: p = ZZ['x']([1,2,3])
sage: p.change_ring(QQ['x']).degree()
0
sage: p.change_ring(SR).degree()
0

CC: @rwst

Component: commutative algebra

Author: Vincent Delecroix

Branch/Commit: 319bb43

Reviewer: Julian Rüth

Issue created by migration from https://trac.sagemath.org/ticket/25022

@nbruin
Copy link
Contributor

nbruin commented Mar 23, 2018

comment:1

Note that change_ring is supposed to change the base ring. For instance, we have:

sage: p.change_ring(QQ['y']).degree()
2
sage: p.change_ring(QQ['y']).parent()
Univariate Polynomial Ring in x over Univariate Polynomial Ring in y over Rational Field

In your case, you basically end up with

sage: QQ['x']['x'](p)
3*x^2 + 2*x + 1

and as it goes with coercion, the x of p coerces as low in the tower as possible.

Perhaps you'd prefer the result

sage: QQ['x']['x'](p)
3*x^2 + 2*x + 1

There's some work to be done, though:

sage: R.<x,y>=ZZ[]
sage: f=x^2+y^2
sage: f.change_ring(QQ['x','y']).monomials()
[1]
sage: f.change_ring(QQ['x']).monomials()
[x^2, y^2]
sage: f.change_ring(QQ['y']).monomials()
[x^2, y^2]

which does match

sage: QQ['x']['x','y'](f).monomials()
[x^2, y^2]
sage: QQ['x','y']['x','y'](f).monomials()
[1]

but doesn't seem to fit the pattern of "coerce 'x' as low as possible. But then, following that rule consistently might not be such a good idea.

sage: QQ['x']['x'](QQ['x']['x'].0).degree()
1

@videlec
Copy link
Contributor Author

videlec commented Mar 23, 2018

comment:2

I think that change_ring on elements should uniformly be

def change_ring(self, R):
    return self.parent().change_ring(R)(list(self))

I don't plan to change anything to the _element_constructor_ which is definitely allowed to behave differently.

@videlec
Copy link
Contributor Author

videlec commented Mar 23, 2018

Commit: 319bb43

@videlec
Copy link
Contributor Author

videlec commented Mar 23, 2018

Branch: u/vdelecroix/25022

@videlec
Copy link
Contributor Author

videlec commented Mar 23, 2018

Author: Vincent Delecroix

@videlec
Copy link
Contributor Author

videlec commented Mar 23, 2018

New commits:

319bb43fix change_ring for polynomials

@saraedum
Copy link
Member

comment:4

Changes look good to me. nbruin, do you agree?

@saraedum
Copy link
Member

Reviewer: Julian Rüth

@rwst
Copy link

rwst commented Mar 25, 2018

comment:5

Patchbot is green, too.

@videlec
Copy link
Contributor Author

videlec commented Apr 3, 2018

comment:6

ping

@saraedum
Copy link
Member

saraedum commented Apr 4, 2018

comment:7

Since nbruin has not reacted, let's assume that he does at least not strongly disagree…

@vbraun
Copy link
Member

vbraun commented May 12, 2018

Changed branch from u/vdelecroix/25022 to 319bb43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants