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:  sage8.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: 
Description (last modified by )
From https://groups.google.com/forum/?hl=en#!topic/sagedevel/hwm7V8S3hE
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
Summary:  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 

comment:2 Changed 7 years ago by
Description:  modified (diff) 

comment:3 Changed 7 years ago by
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
Authors:  → Ralf Stephan 

Commit:  → 0086ec09a89cbdf71ba5b718818dd2de324e892a 
Status:  new → needs_review 
comment:5 Changed 7 years ago by
Commit:  0086ec09a89cbdf71ba5b718818dd2de324e892a → 92d9762ad28880ef81a4781a0eee34b50accc1c6 

Branch pushed to git repo; I updated commit sha1. New commits:
92d9762  18970: fix

comment:6 Changed 7 years ago by
Status:  needs_review → needs_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 cachingwarnings. 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
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).
comment:8 followup: 9 Changed 7 years ago by
Is this a onetime comment, or do you plan to review to the end?
comment:9 Changed 7 years ago by
Replying to rws:
Is this a onetime 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 sagedevel and looked into the ticket and decided to give some comments.
comment:10 Changed 6 years ago by
Milestone:  sage6.9 → sage7.4 

comment:11 Changed 6 years ago by
Status:  needs_info → needs_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.
comment:12 Changed 6 years ago by
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
Branch:  u/rws/always_simplify_log_a_m_a__to_m_for_any_a_m_coercible_to_integer → u/rws/189701 

comment:14 Changed 6 years ago by
Commit:  92d9762ad28880ef81a4781a0eee34b50accc1c6 → 11a81ee44404bf5baa8d2de942796f44f785de4c 

Dependencies:  → pynac0.6.91 
comment:15 Changed 6 years ago by
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
Description:  modified (diff) 

Summary:  always simplify log(a^m,a) to m for any a,m coercible to Integer → Log function bugs with int and negative base 
comment:17 Changed 6 years ago by
Branch:  u/rws/189701 

Commit:  11a81ee44404bf5baa8d2de942796f44f785de4c 
Dependencies:  pynac0.6.91 → pynac0.6.91, #21517, #21518 
comment:18 Changed 6 years ago by
Branch:  → u/rws/log_function_bugs_with_int_and_negative_base 

comment:19 Changed 6 years ago by
Commit:  → 02427f6475206379a32c0ac9119d83d7832a34c4 

Dependencies:  pynac0.6.91, #21517, #21518 → #21623, #21517, #21518 
New commits:
02427f6  redo log function

comment:20 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:21 Changed 6 years ago by
Commit:  02427f6475206379a32c0ac9119d83d7832a34c4 → 456cc14d40a5dd9f1e7924bea9b97e097dd10b5d 

comment:22 Changed 6 years ago by
Summary:  Log function bugs with int and negative base → Log function and documentation revamp 

comment:23 Changed 6 years ago by
Branch:  u/rws/log_function_bugs_with_int_and_negative_base → u/rws/18970 

comment:24 Changed 6 years ago by
Branch:  u/rws/18970 → u/rws/189702 

comment:25 Changed 6 years ago by
Commit:  456cc14d40a5dd9f1e7924bea9b97e097dd10b5d → 838c1fd515514355bc2ad86a866514667109b60d 

comment:26 Changed 6 years ago by
Milestone:  sage7.4 → sage8.0 

comment:27 Changed 5 years ago by
Commit:  838c1fd515514355bc2ad86a866514667109b60d → 3c340011c02cd3881b6661dc2114a1696ad81c8e 

Branch pushed to git repo; I updated commit sha1. New commits:
3c34001  Merge branch 'develop' into t/18970/189702

comment:28 Changed 5 years ago by
Milestone:  sage8.0 → sage8.1 

comment:30 Changed 5 years ago by
Commit:  3c340011c02cd3881b6661dc2114a1696ad81c8e → e46bc475d7e399d4e4d5c9785a0e1e84335a0f59 

comment:31 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:32 Changed 5 years ago by
Status:  needs_review → needs_work 

Needs another rebase over beta9.
comment:33 Changed 5 years ago by
Commit:  e46bc475d7e399d4e4d5c9785a0e1e84335a0f59 → 7ab331ecfa18257d276d66b8eeda5d01616e54c5 

Branch pushed to git repo; I updated commit sha1. New commits:
7ab331e  Merge branch 'develop' into t/18970/189702

comment:34 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:35 Changed 5 years ago by
Commit:  7ab331ecfa18257d276d66b8eeda5d01616e54c5 → b3b9a525146b9a32e0c2e1bc77b1883255e239ec 

Branch pushed to git repo; I updated commit sha1. New commits:
b3b9a52  18970: fix log usage

comment:36 Changed 5 years ago by
Branch:  u/rws/189702 → u/rws/189703 

comment:37 Changed 5 years ago by
Commit:  b3b9a525146b9a32e0c2e1bc77b1883255e239ec → b0486c8b520b278960485499f2b46fdacc2fce83 

Fixes were added, cruft removed, and it all squashed.
New commits:
b0486c8  18970: Log function and documentation revamp

comment:38 Changed 5 years ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → positive_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:40 Changed 5 years ago by
Branch:  u/rws/189703 → b0486c8b520b278960485499f2b46fdacc2fce83 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
18970: always simplify log(a^m,a) if possible