Opened 4 years ago

Closed 3 years ago

#21429 closed defect (fixed)

Rational log not working

Reported by: rws Owned by:
Priority: major Milestone: sage-7.6
Component: numerical Keywords:
Cc: Merged in:
Authors: Ralf Stephan Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 8c2ad23 (Commits) Commit: 8c2ad23abc73ca6af5288385dacee7e959dce841
Dependencies: #21517 Stopgaps:

Description

sage: ZZ(8).log(2)
3

sage: QQ(8/27).log(2/3)
...
AttributeError: 'sage.rings.rational.Rational' object has no attribute 'log'

This can be done fast using mpz_remove (or ZZ.log) on numerator and denominator.

Change History (14)

comment:1 Changed 4 years ago by rws

  • Component changed from calculus to numerical

comment:2 Changed 4 years ago by rws

  • Dependencies set to #21518

comment:3 Changed 4 years ago by rws

  • Dependencies changed from #21518 to #21517

comment:4 Changed 4 years ago by rws

  • Branch set to u/rws/21429-1

comment:5 Changed 4 years ago by rws

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

New commits:

de1acfa21517: handle ZZ.log(1/n)
4aba8a921517: avoid duplicate computations
a16e1c421429: implement rational log

comment:6 Changed 4 years ago by tscrim

Some micro-optimizations (I slightly care because this could be used in tight loops):

  • You call Rational(m) twice.
  • You call the perfect_power of anum and the like twice.
  • an (and other like variables) are integers, correct? IIRC, a faster way to construct rational numbers is Rational(an, bn).
  • It is likely faster to extract the numerator and denominator than to use a try/except block:
    sage: def foo(q):
    ....:     try:
    ....:         return ZZ(q)
    ....:     except (ValueError, TypeError):
    ....:         return None
    sage: def bar(q):
    ....:     if q.denom() == ZZ.one():
    ....:         return q.numer()
    ....:     else:
    ....:         return None
    sage: q = 4 / 3
    sage: %timeit foo(q)
    The slowest run took 7.89 times longer than the fastest. This could mean that an intermediate result is being cached.
    1000000 loops, best of 3: 1.24 µs per loop
    sage: %timeit bar(q)
    The slowest run took 42.67 times longer than the fastest. This could mean that an intermediate result is being cached.
    1000000 loops, best of 3: 212 ns per loop
    sage: q = 4 / 1
    sage: %timeit foo(q)
    The slowest run took 35.47 times longer than the fastest. This could mean that an intermediate result is being cached.
    1000000 loops, best of 3: 202 ns per loop
    sage: %timeit bar(q)
    The slowest run took 37.56 times longer than the fastest. This could mean that an intermediate result is being cached.
    1000000 loops, best of 3: 292 ns per loop
    
    You might be able to get even a bit more out of this by not going through numer and denom, but using the actual mpir data.

A cosmetic change:

-        -  ``m`` - default: natural log base e
+        -  ``m`` -- (default: natural log base `e`) the base
 
-        -  ``prec`` - integer (default: None): if None, return
+        -  ``prec`` -- integer (default: ``None``); if ``None``, return
            symbolic, else to given bits of precision as in RealField

comment:7 Changed 4 years ago by rws

  • Branch changed from u/rws/21429-1 to u/rws/21429-2

comment:8 Changed 4 years ago by rws

  • Commit changed from a16e1c4df5ed2c522af97e5fd5252e751fc51773 to 997afa8d5662f7537ceac46f4fd61fa3e6b99005

This version also looks much better.


New commits:

997afa821429: rational log

comment:9 Changed 3 years ago by cheuberg

  • Status changed from needs_review to needs_work

failing doctests (see patchbot).

comment:10 Changed 3 years ago by git

  • Commit changed from 997afa8d5662f7537ceac46f4fd61fa3e6b99005 to 2ded5706ad4537258b9b9cf47d7947192b2bc89f

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

95e8048Merge branch 'develop' into t/21429/21429-2
2ded57021429: fix doctest fails

comment:11 Changed 3 years ago by rws

  • Milestone changed from sage-7.4 to sage-7.6
  • Status changed from needs_work to needs_review

comment:12 Changed 3 years ago by tscrim

  • Branch changed from u/rws/21429-2 to u/tscrim/rational_log_fix-21429
  • Commit changed from 2ded5706ad4537258b9b9cf47d7947192b2bc89f to 8c2ad23abc73ca6af5288385dacee7e959dce841
  • Reviewers set to Travis Scrimshaw

Sorry for having this fall off my radar. I made some fixes to the doc and some other small cosmetic changes. If you agree, then you can set a positive review.


New commits:

8c2ad23Docstring and cosmetic changes.

comment:13 Changed 3 years ago by rws

  • Status changed from needs_review to positive_review

Thanks!

comment:14 Changed 3 years ago by vbraun

  • Branch changed from u/tscrim/rational_log_fix-21429 to 8c2ad23abc73ca6af5288385dacee7e959dce841
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.