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

Implement __pow__ in the coercion model #24247

Closed
jdemeyer opened this issue Nov 20, 2017 · 91 comments
Closed

Implement __pow__ in the coercion model #24247

jdemeyer opened this issue Nov 20, 2017 · 91 comments

Comments

@jdemeyer
Copy link

We implement powering in the coercion model. One important difference between powering and other operators is that the most common use case for powering is powering something to an integer exponent.

To deal with this integer powering, we implement an action IntegerPowAction. This action calls a special method _pow_int() on the element. In other words, x ^ n for an integer n becomes x._pow_int(n). We also provide a default implementation of _pow_int for MonoidElement and RingElement which uses the square-and-multiply algorithm implemented in generic_power().

For backward compatibility reasons, we also call this action for elements of IntegerModRing(m). In the future, we may rethink what to do here, see #15709.

Apart from this, powering behaves like other binary operators: coercion to a common parent is done if no action is defined.

Note that the 3-argument version of pow() is not supported in the coercion model. Only specific types like Integer implement it. See also #5082.

Fixing powering for specific parents is not within the scope of this ticket, except where it was needed to fix doctest failures. For example, we fix various serious bugs in powering for RDF such as:

sage: RDF(0) ^ RDF(-1)
0.0
sage: RDF(-1) ^ RDF(2)
NaN

Depends on #24467

Component: coercion

Author: Jeroen Demeyer

Branch/Commit: 74d6700

Reviewer: Travis Scrimshaw, Vincent Delecroix

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

@jdemeyer jdemeyer added this to the sage-8.1 milestone Nov 20, 2017
@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

Changed dependencies from #24244 to #24248

@jdemeyer
Copy link
Author

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 20, 2017

Commit: a18280f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 20, 2017

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

9ad8983Fake Integer interface
04235beNew functions integer_check_long() and integer_check_long_py()
ad34554Fix isinstance(x, int) calls in element.pyx
a18280fImplement `__pow__` in the coercion model

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 20, 2017

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

bb114c9Fake Integer interface
4d76bc1New functions integer_check_long() and integer_check_long_py()
9c579c7Fix isinstance(x, int) calls in element.pyx
8b194fbImplement `__pow__` in the coercion model

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 20, 2017

Changed commit from a18280f to 8b194fb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 21, 2017

Changed commit from 8b194fb to 54eb896

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 21, 2017

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

54eb896Implement `__pow__` in the coercion model

@jdemeyer
Copy link
Author

Changed dependencies from #24248 to #24248, #24259

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 21, 2017

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

5e03135Implement `__pow__` in the coercion model

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 21, 2017

Changed commit from 54eb896 to 5e03135

@jdemeyer
Copy link
Author

Changed dependencies from #24248, #24259 to #24248, #24259, #24260

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 21, 2017

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

88eaed5Declare Integer.value as array
5c1f43fDeprecate str ^ Integer
70592a8Fix compiler warning
d3db645Merge commit '88eaed585d8159a4a0763e56caaf3a767d4cb2f0'; commit '5c1f43f861165ff975616f60cab37c92d36d10f6'; commit '70592a850d7677928592c022760d6183bd81364f' into t/24247/implement___pow___in_the_coercion_model
0d3aa63Implement `__pow__` in the coercion model

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 21, 2017

Changed commit from 5e03135 to 0d3aa63

@jdemeyer

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 24, 2017

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

09afe6dImplement `__pow__` in the coercion model

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 24, 2017

Changed commit from 0d3aa63 to 09afe6d

@jdemeyer
Copy link
Author

Changed dependencies from #24248, #24259, #24260 to #24248, #24259, #24260, #24275, #24276

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 5, 2017

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

cabd346Merge tag '8.1.rc4' into t/24247/implement___pow___in_the_coercion_model
b0b4070Various fixes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 5, 2017

Changed commit from 09afe6d to b0b4070

@jdemeyer
Copy link
Author

jdemeyer commented Dec 5, 2017

Changed dependencies from #24248, #24259, #24260, #24275, #24276 to #24248, #24259, #24260, #24275, #24276, #24328

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 5, 2017

Changed commit from b0b4070 to f6d7ba2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 5, 2017

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

c0aca1cNew module to implement generic_power
bcb70b3Merge commit '70592a850d7677928592c022760d6183bd81364f'; commit '88eaed585d8159a4a0763e56caaf3a767d4cb2f0'; commit '5c1f43f861165ff975616f60cab37c92d36d10f6'; commit 'c0aca1c0ac92b95d790f39369a683269efbde530' into t/24247/implement___pow___in_the_coercion_model
f6d7ba2Implement `__pow__` in the coercion model

@jdemeyer
Copy link
Author

jdemeyer commented Dec 6, 2017

Changed dependencies from #24248, #24259, #24260, #24275, #24276, #24328 to #24328, #24259, #24260, #24275, #24276

@jdemeyer
Copy link
Author

jdemeyer commented Jan 9, 2018

comment:62

Replying to @tscrim:

Have you looked at timings?

First of all, it is important to state that this ticket only changes powering for certain parents. It allows to implement powering using the coercion model, but many parents with a custom __pow__ still have that custom __pow__ after applying this ticket and won't use the coercion model. This is case for polynomials for example. So we only look at a few parents where the powering actually uses the new code.

Now the timings. The conclusion seems to be that powering for equal parents is slightly faster than before but that powering with non-equal parents is slower than before. This is because the latter goes through the coercion model now.

ZZZZ

Before:

sage: a = ZZ(2); n = ZZ(2); timeit('a ^ n', number=100000, repeat=20)
100000 loops, best of 20: 122 ns per loop

After:

sage: a = ZZ(2); n = ZZ(2); timeit('a ^ n', number=100000, repeat=20)
100000 loops, best of 20: 110 ns per loop

GF(2n)ZZ

Before:

sage: k.<a> = GF(4, impl="ntl"); n = 2; timeit('a ^ n', number=100000, repeat=20)
100000 loops, best of 20: 377 ns per loop

After:

sage: k.<a> = GF(4, impl="ntl"); n = 2; timeit('a ^ n', number=100000, repeat=20)
100000 loops, best of 20: 607 ns per loop

RDFRDF

Before:

sage: a = RDF(2); n = RDF(2); timeit('a ^ n', number=100000, repeat=20)
100000 loops, best of 20: 139 ns per loop

After:

sage: a = RDF(2); n = RDF(2); timeit('a ^ n', number=100000, repeat=20)
100000 loops, best of 20: 132 ns per loop

RDFZZ

Before:

sage: a = RDF(2); n = ZZ(2); timeit('a ^ n', number=100000, repeat=20)
100000 loops, best of 20: 156 ns per loop

After:

sage: a = RDF(2); n = ZZ(2); timeit('a ^ n', number=100000, repeat=20)
100000 loops, best of 20: 240 ns per loop

idealZZ

Before:

sage: a = ZZ.ideal(2); n = ZZ(2); timeit('a ^ n', number=10000, repeat=20)
10000 loops, best of 20: 13.2 µs per loop

After:

sage: a = ZZ.ideal(2); n = ZZ(2); timeit('a ^ n', number=10000, repeat=20)
10000 loops, best of 20: 14.6 µs per loop

@jdemeyer
Copy link
Author

jdemeyer commented Jan 9, 2018

comment:63

Part of the slowdown is caused by Action.__call__ (see #24500). After optimizing that a bit, I go from

sage: k.<a> = GF(4, impl="ntl"); n = 2; timeit('a ^ n', number=100000, repeat=20)
100000 loops, best of 20: 607 ns per loop

to

sage: k.<a> = GF(4, impl="ntl"); n = 2; timeit('a ^ n', number=100000, repeat=20)
100000 loops, best of 20: 506 ns per loop

and from

sage: a = RDF(2); n = ZZ(2); timeit('a ^ n', number=100000, repeat=20)
100000 loops, best of 20: 240 ns per loop

to

sage: a = RDF(2); n = ZZ(2); timeit('a ^ n', number=100000, repeat=20)
100000 loops, best of 20: 182 ns per loop

@jdemeyer
Copy link
Author

jdemeyer commented Jan 9, 2018

comment:64

Travis, please let me know what you think of the timings and whether #24500 should be a dependency of this ticket. Personally, I think that #24500 will be non-trivial, so I would rather not make it a dependency.

@videlec
Copy link
Contributor

videlec commented Jan 9, 2018

comment:65

I like very much the idea of moving powering inside the coercion model. Though, it is not a binary operator X x X -> X (do you have any example of this beyond NN or RR_+?). I still don't understand why you would need

Apart from this, powering behaves like other binary operators:
coercion to a common parent is done if no action is defined. 

@jdemeyer
Copy link
Author

jdemeyer commented Jan 9, 2018

comment:66

Replying to @videlec:

I like very much the idea of moving powering inside the coercion model. Though, it is not a binary operator X x X -> X (do you have any example of this beyond NN or RR_+?). I still don't understand why you would need

Apart from this, powering behaves like other binary operators:
coercion to a common parent is done if no action is defined. 

Copying from [comment:34]:

I think that coercing to a common parent does make sense because:

  1. It is consistent with all other operators. Often multiplication is treated as an action too. Still, when no action is found, coercion to a common parent is done.

  2. There are several parents where a common parent for powering makes sense, in particular the various incarnations of real/complex numbers and the symbolic ring.

  3. If coercion to a common parent shouldn't be done, then what should be done by default if no action is found?

@tscrim
Copy link
Collaborator

tscrim commented Jan 9, 2018

comment:67

Thank for the timings and explanation. I am the most concerned with the GF(2n)ZZ timings being ~2x slower as I would expect exponentiation to be a fairly common operation. For me personally, it is not important, but I know for the number theorist using Sage, it could have a much bigger impact. However, you know that area of math/Sage better than I, so if you think it will be okay, then no objection from me.

@jdemeyer
Copy link
Author

comment:68

Replying to @tscrim:

Thank for the timings and explanation. I am the most concerned with the GF(2n)ZZ timings being ~2x slower

It's actually only a factor 1.61 slower but that will improve significantly after #24500. Possibly, other optimizations in the coercion model are possible.

It is also important to note that I intentionally took the worst case of GF(4) implemented using NTL. The default implementation for that field size is Givaro, which is not affected by this ticket. Only for GF(2^16) and larger is NTL the default and then the cost of the coercion becomes a much smaller fraction of the actual computation.

@tscrim
Copy link
Collaborator

tscrim commented Jan 10, 2018

comment:69

Okay, then positive review from me.

Does anyone else have any other objections or questions?

@videlec
Copy link
Contributor

videlec commented Jan 11, 2018

comment:70

fine for me

@jdemeyer
Copy link
Author

Reviewer: Travis Scrimshaw, Vincent Delecroix

@vbraun
Copy link
Member

vbraun commented Jan 15, 2018

Changed branch from u/jdemeyer/implement___pow___in_the_coercion_model to 74d6700

@vbraun vbraun closed this as completed in 05825d6 Jan 15, 2018
kryzar pushed a commit to kryzar/sage that referenced this issue Feb 6, 2023
…s Integer

The `absolute_discriminant()` of any number field should be a Sage
`Integer`. This is currently not properly doctested, leading to a subtle
failure in sagemath#24247.

URL: https://trac.sagemath.org/24462
Reported by: jdemeyer
Ticket author(s): Frédéric Chapoton
Reviewer(s): Travis Scrimshaw
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