Skip to content

Log function and documentation revamp #18970

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

Closed
rwst opened this issue Jul 30, 2015 · 49 comments
Closed

Log function and documentation revamp #18970

rwst opened this issue Jul 30, 2015 · 49 comments

Comments

@rwst
Copy link
Contributor

rwst commented Jul 30, 2015

From https://groups.google.com/forum/?hl=en#!topic/sage-devel/hwm-7V8S3hE

sage: log(int(8),2)
log(8)/log(2)
sage: log(8,int(2))
log(8)/log(2)
sage: log(8,2)
3

Also:
sage: log(1/8,2)
log(1/8)/log(2)
sage: ((-1)^(-I*log(3)/pi)).n()
3.00000000000000
sage: log(3,-1)
...
ValueError: m must be positive

Depends on #21623
Depends on #21517
Depends on #21518

Component: symbolics

Author: Ralf Stephan

Branch/Commit: b0486c8

Reviewer: Travis Scrimshaw

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

@rwst rwst added this to the sage-6.9 milestone Jul 30, 2015
@rwst rwst changed the title always simplify log(a^m,a) to m for any m coercible to Integer always simplify log(a^m,a) to m for any a,m coercible to Integer Jul 30, 2015
@rwst

This comment has been minimized.

@rwst
Copy link
Contributor Author

rwst commented Jul 30, 2015

@rwst
Copy link
Contributor Author

rwst commented Jul 30, 2015

Author: Ralf Stephan

@rwst
Copy link
Contributor Author

rwst commented Jul 30, 2015

New commits:

0086ec018970: always simplify log(a^m,a) if possible

@rwst
Copy link
Contributor Author

rwst commented Jul 30, 2015

Commit: 0086ec0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 30, 2015

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

92d976218970: fix

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 30, 2015

Changed commit from 0086ec0 to 92d9762

@dkrenn
Copy link
Contributor

dkrenn commented Jul 30, 2015

comment:6

Before this patch

sage: %timeit log(QQ(8),2)
The slowest run took 5.08 times longer than the fastest. This could mean that an intermediate result is being cached 
10000 loops, best of 3: 39.4 µs per loop
sage: %timeit log(QQ(8),3)
10000 loops, best of 3: 38.1 µs per loop
sage: %timeit log(ZZ(8),3)
10000 loops, best of 3: 32.8 µs per loop
sage: %timeit log(ZZ(8),2)
The slowest run took 14.52 times longer than the fastest. This could mean that an intermediate result is being cached 
1000000 loops, best of 3: 1.12 µs per loop

After

sage: %timeit log(QQ(8),3)
The slowest run took 37.28 times longer than the fastest. This could mean that an intermediate result is being cached 
10000 loops, best of 3: 48.8 µs per loop
sage: %timeit log(QQ(8),2)
The slowest run took 8.69 times longer than the fastest. This could mean that an intermediate result is being cached 
100000 loops, best of 3: 5.43 µs per loop
sage: %timeit log(ZZ(8),3)
10000 loops, best of 3: 48.4 µs per loop
sage: %timeit log(ZZ(8),2)
The slowest run took 8.80 times longer than the fastest. This could mean that an intermediate result is being cached 
100000 loops, best of 3: 5.34 µs per loop

I've repeated (differently) it due to the caching-warnings. Before

sage: timeit('log(QQ(8),3)', number=1, repeat=1)
1 loops, best of 1: 191 µs per loop
sage: timeit('log(QQ(8),2)', number=1, repeat=1)
1 loops, best of 1: 150 µs per loop
sage: timeit('log(ZZ(8),3)', number=1, repeat=1)
1 loops, best of 1: 125 µs per loop
sage: timeit('log(ZZ(8),2)', number=1, repeat=1)
1 loops, best of 1: 24.1 µs per loop

After

sage: timeit('log(QQ(8),3)', number=1, repeat=1)
1 loops, best of 1: 1.93 ms per loop
sage: timeit('log(QQ(8),2)', number=1, repeat=1)
1 loops, best of 1: 45.1 µs per loop
sage: timeit('log(ZZ(8),3)', number=1, repeat=1)
1 loops, best of 1: 158 µs per loop
sage: timeit('log(ZZ(8),2)', number=1, repeat=1)
1 loops, best of 1: 45.1 µs per loop

In particular log(QQ(8),3) becomes much slower (and only log(QQ(8),2) gains speed).

@dkrenn
Copy link
Contributor

dkrenn commented Jul 30, 2015

comment:7
sage: log(QQ(8),2).parent()
Integer Ring

which is a different behavior compared to the common/similar arithmetic operations (e.g. (QQ(2)^ZZ(1)).parent() is Rational Field and not Integer Ring) and I would expect that the logarithm of a rational is again a rational (if possible) or something where QQ coerces into (but not out of something which coerces into QQ).

@rwst
Copy link
Contributor Author

rwst commented Jul 30, 2015

comment:8

Is this a one-time comment, or do you plan to review to the end?

@dkrenn
Copy link
Contributor

dkrenn commented Jul 30, 2015

comment:9

Replying to @rwst:

Is this a one-time comment, or do you plan to review to the end?

Does this make a difference on how to react to my comment? ;) ....or more seriously: I am not sure if I feel qualified enough to review a change in the log-function. I just saw the discussion on sage-devel and looked into the ticket and decided to give some comments.

@nexttime nexttime mannequin modified the milestones: sage-6.9, sage-7.4 Aug 29, 2016
@rwst
Copy link
Contributor Author

rwst commented Aug 30, 2016

comment:11

The code at hand unconditionally tries to coerce the arguments into ZZ,QQ instead of checking the type first. Apart from the speed issue it also would benefit from moving completely into GiNaC: atm some cases are intercepted unnecessarily in Function_log.__call__ which itself awkwardly calls Pynac.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 30, 2016

comment:12

I guess some of the speed regression is due to the at least partially redundant checking whether arg == base**v, and of course now in addition computing the valuation first in case the latter does not hold. (At least it seemed to when I looked at it yesterday...)

Not sure how much the probably superfluous coercion contributes.

@rwst
Copy link
Contributor Author

rwst commented Sep 7, 2016

@rwst
Copy link
Contributor Author

rwst commented Sep 7, 2016

Dependencies: pynac-0.6.91

@rwst
Copy link
Contributor Author

rwst commented Sep 7, 2016

comment:14

New branch depends on Pynac git master.


New commits:

11a81ee18970: add two-arg log()

@rwst
Copy link
Contributor Author

rwst commented Sep 7, 2016

Changed commit from 92d9762 to 11a81ee

@rwst
Copy link
Contributor Author

rwst commented Sep 7, 2016

comment:15

Timings with this branch. Before:

sage: %timeit log(QQ(8),2)
The slowest run took 10.96 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 40.1 µs per loop
sage: %timeit log(QQ(8),2)
10000 loops, best of 3: 40.2 µs per loop
sage: %timeit log(QQ(8),3)
10000 loops, best of 3: 40.5 µs per loop
sage: %timeit log(ZZ(8),3)
10000 loops, best of 3: 34.4 µs per loop
sage: %timeit log(ZZ(8),2)
The slowest run took 19.11 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 886 ns per loop

After:

sage: %timeit log(QQ(8),2)
The slowest run took 15.53 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 26.7 µs per loop
sage: %timeit log(QQ(8),3)
The slowest run took 10.80 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 26.8 µs per loop
sage: %timeit log(ZZ(8),3)
10000 loops, best of 3: 30.2 µs per loop
sage: %timeit log(ZZ(8),2)
The slowest run took 17.37 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 686 ns per loop

The single run measurements vary so much from run to run that I refrain from giving them here. The After values of timeit('log(QQ(8),3)', number=1, repeat=1) are however in the same range as the Before values (as well as the others).

@rwst rwst added this to the sage-8.1 milestone Aug 24, 2017
@fchapoton
Copy link
Contributor

comment:29

does not apply

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2017

Changed commit from 3c34001 to e46bc47

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2017

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

94cc4a5Merge branch 'develop' into t/18970/18970-2
e46bc4718970: fix and robustify doctests

@tscrim
Copy link
Collaborator

tscrim commented Oct 23, 2017

comment:32

Needs another rebase over beta9.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 24, 2017

Changed commit from e46bc47 to 7ab331e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 24, 2017

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

7ab331eMerge branch 'develop' into t/18970/18970-2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 24, 2017

Changed commit from 7ab331e to b3b9a52

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 24, 2017

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

b3b9a5218970: fix log usage

@rwst
Copy link
Contributor Author

rwst commented Oct 24, 2017

Changed branch from u/rws/18970-2 to u/rws/18970-3

@rwst
Copy link
Contributor Author

rwst commented Oct 24, 2017

Changed commit from b3b9a52 to b0486c8

@rwst
Copy link
Contributor Author

rwst commented Oct 24, 2017

comment:37

Fixes were added, cruft removed, and it all squashed.


New commits:

b0486c818970: Log function and documentation revamp

@tscrim
Copy link
Collaborator

tscrim commented Oct 25, 2017

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Oct 25, 2017

comment:38

LGTM. Timings on my laptop with branch:

sage: %timeit log(QQ(8),2)
The slowest run took 29.31 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 1.57 µs per loop
sage: %timeit log(QQ(8),3)
The slowest run took 4.03 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 35.5 µs per loop
sage: %timeit log(ZZ(8),3)
10000 loops, best of 3: 33 µs per loop
sage: %timeit log(ZZ(8),2)
The slowest run took 16.93 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 887 ns per loop

vs. with develop:

sage: %timeit log(QQ(8),2)
The slowest run took 14987.26 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 1.6 µs per loop
sage: %timeit log(QQ(8),3)
10000 loops, best of 3: 43.2 µs per loop
sage: %timeit log(ZZ(8),3)
The slowest run took 4.42 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 38.9 µs per loop
sage: %timeit log(ZZ(8),2)
The slowest run took 14.23 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 1.06 µs per loop

@rwst
Copy link
Contributor Author

rwst commented Oct 26, 2017

comment:39

Thanks.

@vbraun
Copy link
Member

vbraun commented Oct 29, 2017

Changed branch from u/rws/18970-3 to b0486c8

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