Opened 7 years ago

Closed 5 years ago

#18970 closed defect (fixed)

Log function and documentation revamp

Reported by: Ralf Stephan Owned by:
Priority: major Milestone: sage-8.1
Component: symbolics Keywords:
Cc: Merged in:
Authors: Ralf Stephan Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: b0486c8 (Commits, GitHub, GitLab) Commit: b0486c8b520b278960485499f2b46fdacc2fce83
Dependencies: #21623, #21517, #21518 Stopgaps:

Status badges

Description (last modified by Ralf Stephan)

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

Change History (40)

comment:1 Changed 7 years ago by Ralf Stephan

Summary: always simplify log(a^m,a) to m for any m coercible to Integeralways simplify log(a^m,a) to m for any a,m coercible to Integer

comment:2 Changed 7 years ago by Ralf Stephan

Description: modified (diff)

comment:3 Changed 7 years ago by Ralf Stephan

Branch: u/rws/always_simplify_log_a_m_a__to_m_for_any_a_m_coercible_to_integer

comment:4 Changed 7 years ago by Ralf Stephan

Authors: Ralf Stephan
Commit: 0086ec09a89cbdf71ba5b718818dd2de324e892a
Status: newneeds_review

New commits:

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

comment:5 Changed 7 years ago by git

Commit: 0086ec09a89cbdf71ba5b718818dd2de324e892a92d9762ad28880ef81a4781a0eee34b50accc1c6

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

92d976218970: fix

comment:6 Changed 7 years ago by Daniel Krenn

Status: needs_reviewneeds_info

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).

comment:7 Changed 7 years ago by Daniel Krenn

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 (and not converted to an integer).

Version 0, edited 7 years ago by Daniel Krenn (next)

comment:8 Changed 7 years ago by Ralf Stephan

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

comment:9 in reply to:  8 Changed 7 years ago by Daniel Krenn

Replying to rws:

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.

comment:10 Changed 6 years ago by Leif Leonhardy

Milestone: sage-6.9sage-7.4

comment:11 Changed 6 years ago by Ralf Stephan

Status: needs_infoneeds_work

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.

Last edited 6 years ago by Ralf Stephan (previous) (diff)

comment:12 Changed 6 years ago by Leif Leonhardy

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.

comment:13 Changed 6 years ago by Ralf Stephan

Branch: u/rws/always_simplify_log_a_m_a__to_m_for_any_a_m_coercible_to_integeru/rws/18970-1

comment:14 Changed 6 years ago by Ralf Stephan

Commit: 92d9762ad28880ef81a4781a0eee34b50accc1c611a81ee44404bf5baa8d2de942796f44f785de4c
Dependencies: pynac-0.6.91

New branch depends on Pynac git master.


New commits:

11a81ee18970: add two-arg log()

comment:15 Changed 6 years ago by Ralf Stephan

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).

comment:16 Changed 6 years ago by Ralf Stephan

Description: modified (diff)
Summary: always simplify log(a^m,a) to m for any a,m coercible to IntegerLog function bugs with int and negative base

comment:17 Changed 6 years ago by Ralf Stephan

Branch: u/rws/18970-1
Commit: 11a81ee44404bf5baa8d2de942796f44f785de4c
Dependencies: pynac-0.6.91pynac-0.6.91, #21517, #21518

comment:18 Changed 6 years ago by Ralf Stephan

Branch: u/rws/log_function_bugs_with_int_and_negative_base

comment:19 Changed 6 years ago by Ralf Stephan

Commit: 02427f6475206379a32c0ac9119d83d7832a34c4
Dependencies: pynac-0.6.91, #21517, #21518#21623, #21517, #21518

New commits:

02427f6redo log function

comment:20 Changed 6 years ago by Ralf Stephan

Status: needs_workneeds_review

comment:21 Changed 6 years ago by git

Commit: 02427f6475206379a32c0ac9119d83d7832a34c4456cc14d40a5dd9f1e7924bea9b97e097dd10b5d

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

584c4f3Merge branch 'develop' into t/18970/log_function_bugs_with_int_and_negative_base
456cc1418970: doc restructuring to get the most out of typing log?

comment:22 Changed 6 years ago by Ralf Stephan

Summary: Log function bugs with int and negative baseLog function and documentation revamp

comment:23 Changed 6 years ago by Ralf Stephan

Branch: u/rws/log_function_bugs_with_int_and_negative_baseu/rws/18970

comment:24 Changed 6 years ago by Ralf Stephan

Branch: u/rws/18970u/rws/18970-2

comment:25 Changed 6 years ago by Ralf Stephan

Commit: 456cc14d40a5dd9f1e7924bea9b97e097dd10b5d838c1fd515514355bc2ad86a866514667109b60d

Completed coverage.


New commits:

838c1fd18970: Log function and documentation revamp

comment:26 Changed 6 years ago by Ralf Stephan

Milestone: sage-7.4sage-8.0

comment:27 Changed 5 years ago by git

Commit: 838c1fd515514355bc2ad86a866514667109b60d3c340011c02cd3881b6661dc2114a1696ad81c8e

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

3c34001Merge branch 'develop' into t/18970/18970-2

comment:28 Changed 5 years ago by Ralf Stephan

Milestone: sage-8.0sage-8.1

comment:29 Changed 5 years ago by Frédéric Chapoton

Status: needs_reviewneeds_work

does not apply

comment:30 Changed 5 years ago by git

Commit: 3c340011c02cd3881b6661dc2114a1696ad81c8ee46bc475d7e399d4e4d5c9785a0e1e84335a0f59

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

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

comment:31 Changed 5 years ago by Ralf Stephan

Status: needs_workneeds_review

comment:32 Changed 5 years ago by Travis Scrimshaw

Status: needs_reviewneeds_work

Needs another rebase over beta9.

comment:33 Changed 5 years ago by git

Commit: e46bc475d7e399d4e4d5c9785a0e1e84335a0f597ab331ecfa18257d276d66b8eeda5d01616e54c5

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

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

comment:34 Changed 5 years ago by Ralf Stephan

Status: needs_workneeds_review

comment:35 Changed 5 years ago by git

Commit: 7ab331ecfa18257d276d66b8eeda5d01616e54c5b3b9a525146b9a32e0c2e1bc77b1883255e239ec

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

b3b9a5218970: fix log usage

comment:36 Changed 5 years ago by Ralf Stephan

Branch: u/rws/18970-2u/rws/18970-3

comment:37 Changed 5 years ago by Ralf Stephan

Commit: b3b9a525146b9a32e0c2e1bc77b1883255e239ecb0486c8b520b278960485499f2b46fdacc2fce83

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


New commits:

b0486c818970: Log function and documentation revamp

comment:38 Changed 5 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

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

comment:39 Changed 5 years ago by Ralf Stephan

Thanks.

comment:40 Changed 5 years ago by Volker Braun

Branch: u/rws/18970-3b0486c8b520b278960485499f2b46fdacc2fce83
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.