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

rational powers in ZZ[X] and QQ[X] #20086

Closed
cheuberg opened this issue Feb 19, 2016 · 127 comments
Closed

rational powers in ZZ[X] and QQ[X] #20086

cheuberg opened this issue Feb 19, 2016 · 127 comments

Comments

@cheuberg
Copy link
Contributor

Until now,

sage: R.<x> = ZZ[]
sage: R(1)^(1/2)
Traceback (most recent call last):
...
TypeError: rational is not an integer

because only integer exponents were allowed for polynomials.

Implement arbitrary powers of constant polynomials by handing over to the rational field.

This was originally observed in the asymptotic ring:

sage: P.<R> = QQ[]
sage: A.<Z> = AsymptoticRing('T^QQ', P)
sage: sqrt(Z)
Traceback (most recent call last):
...
ArithmeticError: Cannot take T to the exponent 1/2 in Exact Term Monoid T^QQ
with coefficients in Univariate Polynomial Ring in R over Rational Field
since its coefficient 1 cannot be taken to this exponent.
> *previous* TypeError: rational is not an integer

follow up: #20571

CC: @behackl @dkrenn

Component: basic arithmetic

Author: Clemens Heuberger, Vincent Delecroix, Benjamin Hackl

Branch/Commit: 5fc1027

Reviewer: Benjamin Hackl, Vincent Delecroix

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

@cheuberg
Copy link
Contributor Author

Branch: u/cheuberg/polynomials/power

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2016

Commit: cc49098

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2016

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

cc49098Trac #20086: powers of constant polynomials

@cheuberg

This comment has been minimized.

@cheuberg
Copy link
Contributor Author

Author: Clemens Heuberger

@cheuberg cheuberg changed the title Asymptotic ring: fix exponentiation QQ[X]: allow arbitrary powers of constant polynomials Feb 19, 2016
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2016

Changed commit from cc49098 to 25fdd0c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2016

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

25fdd0cTrac #20086: powers of constant polynomials over ZZ

@cheuberg cheuberg changed the title QQ[X]: allow arbitrary powers of constant polynomials ZZ[X], QQ[X]: allow arbitrary powers of constant polynomials Feb 19, 2016
@nbruin
Copy link
Contributor

nbruin commented Feb 20, 2016

comment:6

The fact that for a in QQ, the value of a^(1/2) has its parent depending on the actual value of a is a compromise for novice use (in calculus etc.) Normally, one would raise an error if a^(1/2) does not lie in QQ. Certainly, promoting to SR is a very bad choice for anything else than novice use.

I see you take the effort of trying to put the result back in the original parent. Don't you think it's better to force that? i.e., raise an error if it doesn't work, rather than give a result back in SR?

The problem is that if someone is computing with polynomials, it's almost certainly not desired to end up in SR (where things like QQ['x']['x'] get squashed), and if an error isn't raised, it's very hard to detect that it happened.

Also, if you're implementing the (partial) map a:->a^(n/m) , why not extend it properly? When
(QQ['x'](4))^(1/2) works, why should QQ['x'](x<sup>2)</sup>(1/2) fail?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2016

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

6217beeTrac #20085: raise exception instead of moving to SR

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2016

Changed commit from 25fdd0c to 6217bee

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2016

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

918499cTrac #20086: raise exception instead of moving to SR

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2016

Changed commit from 6217bee to 918499c

@cheuberg
Copy link
Contributor Author

comment:9

Replying to @nbruin:

The fact that for a in QQ, the value of a^(1/2) has its parent depending on the actual value of a is a compromise for novice use (in calculus etc.) Normally, one would raise an error if a^(1/2) does not lie in QQ. Certainly, promoting to SR is a very bad choice for anything else than novice use.

I see you take the effort of trying to put the result back in the original parent. Don't you think it's better to force that? i.e., raise an error if it doesn't work, rather than give a result back in SR?

The problem is that if someone is computing with polynomials, it's almost certainly not desired to end up in SR (where things like QQ['x']['x'] get squashed), and if an error isn't raised, it's very hard to detect that it happened.

very valid points, thank you. I now raise an exception.

Also, if you're implementing the (partial) map a:->a^(n/m) , why not extend it properly? When
(QQ['x'](4))^(1/2) works, why should QQ['x'](x<sup>2)</sup>(1/2) fail?

I needed a fix a bug which did bite me somewhere else, see initial description. Of course, it would be nice to have that, too; but I'd prefer to leave that to a follow-up ticket and to have this functionality here soon.

@behackl
Copy link
Member

behackl commented Mar 8, 2016

Changed branch from u/cheuberg/polynomials/power to u/behackl/polynomials/power

@behackl
Copy link
Member

behackl commented Mar 8, 2016

comment:11

I've fixed the segmentation fault from rings/integer.pyx and implemented the case of constant^constant.

Also, +1 for extending this functionality on a follow-up ticket; I'd also like to have this in as soon as possible.

I've reviewed your changes, please cross-review. If you are satisfied and if there are no other objections, I'd set this to positive_review.


New commits:

caa02c8Merge tag '7.1.beta6' into HEAD
3f44399fix segmentation fault
5934ed1fix (constant poly)^(constant poly)
e6e1c81add doctests

@behackl
Copy link
Member

behackl commented Mar 8, 2016

Changed commit from 918499c to e6e1c81

@behackl
Copy link
Member

behackl commented Mar 8, 2016

Reviewer: Benjamin Hackl

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 9, 2016

Changed commit from e6e1c81 to 87a22b6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 9, 2016

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

87a22b6fix "referenced before assignment"

@cheuberg
Copy link
Contributor Author

cheuberg commented Mar 9, 2016

Changed branch from u/behackl/polynomials/power to u/cheuberg/polynomials/power

@cheuberg
Copy link
Contributor Author

cheuberg commented Mar 9, 2016

Changed commit from 87a22b6 to 14f7efa

@behackl
Copy link
Member

behackl commented May 7, 2016

comment:81

Replying to @videlec:

sage: R.<x> = ZZ[]
sage: parent(R.one().nth_root(3))
Integer Ring

Fixed, will push in a moment.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 7, 2016

Changed commit from e8adfb1 to 2c46e08

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 7, 2016

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

2c46e08return one from original parent, not base ring

@videlec
Copy link
Contributor

videlec commented May 7, 2016

comment:84

Patchbot reports doctest failure due to the change in the error message.

@videlec
Copy link
Contributor

videlec commented May 8, 2016

comment:85

I have an implementation for a much faster implementation of n-th root at #20571.

@videlec

This comment has been minimized.

@videlec

This comment has been minimized.

@videlec

This comment has been minimized.

@behackl
Copy link
Member

behackl commented May 8, 2016

comment:88

Replying to @videlec:

Patchbot reports doctest failure due to the change in the error message.

Indeed, and this can be fixed straightforward. However, there is (once again ...) a slight inconvenience: there are superfluous parentheses in, e.g.

sage: P.<x> = ZZ[]
sage: P(4).nth_root(3)
Traceback (most recent call last):
...
ValueError: (4)^(1/3) does not lie in ...

or

sage: x.nth_root(3)
Traceback (most recent call last):
...
ValueError: (x)^(1/3) does not lie in ...

Should we live with that? Fixing it would require querying something like self.is_constant() and self in self.parent().gens() or so.

EDIT: self.is_constant() would not work:

sage: P.<x> = ZZ[]
sage: Q.<y> = P[]
sage: Q(x+1).is_constant()
True

@nbruin
Copy link
Contributor

nbruin commented May 8, 2016

comment:89

I would think most people would prioritize #20571 over such typographical issues, so I wouldn't sweat too hard over parentheses at this point. Another issue: while it's nice to have informative error messages, error strings that cost a lot to be constructed only to be caught by an "except" statement can mean a performance penalty. This is more an issue in python than in many other languages because there's a culture in python (and perhaps even more so in some parts of sage) to use exceptions for regular program flow control, not just for error conditions.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 8, 2016

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

5fc1027fix doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 8, 2016

Changed commit from 2c46e08 to 5fc1027

@behackl
Copy link
Member

behackl commented May 8, 2016

comment:91

Replying to @nbruin:

I would think most people would prioritize #20571 over such typographical issues, so I wouldn't sweat too hard over parentheses at this point. Another issue: while it's nice to have informative error messages, error strings that cost a lot to be constructed only to be caught by an "except" statement can mean a performance penalty. This is more an issue in python than in many other languages because there's a culture in python (and perhaps even more so in some parts of sage) to use exceptions for regular program flow control, not just for error conditions.

Yes, that's my feeling too. While it is somehow unfortunate, I can very well live with it.

I've fixed the doctests and tested the entire src/sage/rings/polynomial-directory; let's see what the patchbot thinks.

@videlec
Copy link
Contributor

videlec commented May 10, 2016

comment:93

Let's move to #20571.

@vbraun
Copy link
Member

vbraun commented May 17, 2016

Changed branch from public/20086 to 5fc1027

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