Opened 7 years ago

Last modified 5 years ago

#18970 closed defect

Log function bugs with int and negative base — at Version 16

Reported by: rws Owned by:
Priority: major Milestone: sage-8.1
Component: symbolics Keywords:
Cc: Merged in:
Authors: Ralf Stephan Reviewers:
Report Upstream: N/A Work issues:
Branch: u/rws/18970-1 (Commits, GitHub, GitLab) Commit: 11a81ee44404bf5baa8d2de942796f44f785de4c
Dependencies: pynac-0.6.91 Stopgaps:

Status badges

Description (last modified by rws)

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 (16)

comment:1 Changed 7 years ago by rws

  • Summary changed from always simplify log(a^m,a) to m for any m coercible to Integer to always simplify log(a^m,a) to m for any a,m coercible to Integer

comment:2 Changed 7 years ago by rws

  • Description modified (diff)

comment:3 Changed 7 years ago by rws

  • Branch set to u/rws/always_simplify_log_a_m_a__to_m_for_any_a_m_coercible_to_integer

comment:4 Changed 7 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to 0086ec09a89cbdf71ba5b718818dd2de324e892a
  • Status changed from new to needs_review

New commits:

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

comment:5 Changed 7 years ago by git

  • Commit changed from 0086ec09a89cbdf71ba5b718818dd2de324e892a to 92d9762ad28880ef81a4781a0eee34b50accc1c6

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

92d976218970: fix

comment:6 Changed 7 years ago by dkrenn

  • Status changed from needs_review to 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 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 dkrenn

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

Last edited 7 years ago by dkrenn (previous) (diff)

comment:8 follow-up: Changed 7 years ago by rws

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 dkrenn

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

  • Milestone changed from sage-6.9 to sage-7.4

comment:11 Changed 6 years ago by rws

  • Status changed from needs_info to 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.

Last edited 6 years ago by rws (previous) (diff)

comment:12 Changed 6 years ago by leif

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 rws

  • Branch changed from u/rws/always_simplify_log_a_m_a__to_m_for_any_a_m_coercible_to_integer to u/rws/18970-1

comment:14 Changed 6 years ago by rws

  • Commit changed from 92d9762ad28880ef81a4781a0eee34b50accc1c6 to 11a81ee44404bf5baa8d2de942796f44f785de4c
  • Dependencies set to 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 rws

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 rws

  • Description modified (diff)
  • Summary changed from always simplify log(a^m,a) to m for any a,m coercible to Integer to Log function bugs with int and negative base
Note: See TracTickets for help on using tickets.