Opened 7 years ago

Closed 5 years ago

#19444 closed defect (fixed)

sage.misc.functional.log(float(3)) raises an AttributeError

Reported by: slabbe Owned by:
Priority: major Milestone: sage-8.1
Component: basic arithmetic Keywords:
Cc: tscrim, jdemeyer, jhpalmieri Merged in:
Authors: Frédéric Chapoton Reviewers: Sébastien Labbé
Report Upstream: N/A Work issues:
Branch: d2f7962 (Commits, GitHub, GitLab) Commit: d2f7962a6ee8a2e6e0b174083a44b4ed067b5785
Dependencies: Stopgaps:

Status badges

Description

import_statements suggest to import log this way:

sage: import_statements('log')
# **Warning**: distinct objects with name 'log' in:
#   - sage.functions.log
#   - math
#   - sage.functions
#   - sage.misc.functional
from sage.misc.functional import log

While those works:

sage: math.log(float(3))
1.0986122886681098
sage: sage.functions.log.log(float(3))
1.0986122886681098

This one raises an AttributeError:

sage: sage.misc.functional.log(float(3))
Traceback (most recent call last):
...
AttributeError: 'sage.rings.real_double.RealDoubleElement' object has no attribute '_log_base'

A subquestion is why do we have two implementations of log?

Change History (26)

comment:1 Changed 5 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch set to u/chapoton/19444
  • Cc tscrim jdemeyer jhpalmieri added
  • Commit set to f4c012de47d26ab2fabb24266805ac6dccd5a8cb
  • Milestone changed from sage-6.10 to sage-8.1
  • Status changed from new to needs_review

Done. This should be an easy review.

NOTA BENE: Getting rid of this log function (in favor of the one in functions.log) should be done in another ticket. I tried to do that, but sutmbled on the infamous import hell (import cycles everywhere).


New commits:

f4c012dfixing the behaviour of misc.functional.log

comment:2 Changed 5 years ago by vdelecroix

This implementation is very weak. How can you assume that this is supposed to be the real logarithm?

sage: sage.misc.functional.log(complex(3j))
Traceback (most recent call last):
...
TypeError: can't convert complex to float

versus

sage: sage.misc.functional.log(CDF(0,3))
1.0986122886681098 + 1.5707963267948966*I

comment:3 Changed 5 years ago by chapoton

There is no need to make another perfect log, there is one already in functions.log, which is the one we provide as "log" in the global namespace. We should fix the issue raised here. And later get rid of this function, which is only used 10 times or so.

comment:4 Changed 5 years ago by vdelecroix

What is the point of fixing an issue on a useless function? If I open a ticket for the complex case will you fix it ;-? If useless, the log from functional would better be deprecated and this ticket closed as invalid.

Anyway, the complaint is about import_statements that is not fixed by your branch.

comment:5 Changed 5 years ago by vdelecroix

And indeed, there is something wrong with find_objects_from_name

sage: 
sage: from sage.misc.dev_tools import find_objects_from_name
sage: find_objects_from_name('log')
[<function log at ...>,
 <function log at ...>,
 log,
 <built-in function log>,
 <module 'sage.functions.log' from '/opt/sage/local/lib/python2.7/site-packages/sage/functions/log.pyc'>]

The above is wrong since log is in the global namespace and the answer should have been the log from the global namespace...

comment:6 Changed 5 years ago by vdelecroix

And indeed, globals() is restricted to the module where the function is defined... so that its usage is wrong.

comment:7 Changed 5 years ago by vdelecroix

See #23779 for the fix to import_statements

comment:8 Changed 5 years ago by git

  • Commit changed from f4c012de47d26ab2fabb24266805ac6dccd5a8cb to 02e1fc084228ff509b6927ce82190acb55426caf

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b0aa14ftest
02e1fc0trac 19444 avoid circular imports

comment:9 Changed 5 years ago by chapoton

ok, done

comment:10 Changed 5 years ago by git

  • Commit changed from 02e1fc084228ff509b6927ce82190acb55426caf to b1a85c98b3d957bc23f9002de7db6cc54e52dce0

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

b1a85c9trac 19444 fixing another log import

comment:11 Changed 5 years ago by slabbe

  • Reviewers set to Sébastien Labbé
  • Status changed from needs_review to needs_work

If it turns out that for some reason I have used the bad log function in my code and I now get the following error message:

DeprecationWarning: this version of log is no longer used

I will not understand and I will lose time to understand what is happening.

I much prefer

DeprecationWarning: function sage.misc.functional.log is deprecated, use sage.functions.log or log from the global sage namespace instead

comment:12 Changed 5 years ago by git

  • Commit changed from b1a85c98b3d957bc23f9002de7db6cc54e52dce0 to 465125e6133b5f33493d1f78b98237228bb7dae4

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

465125etrac 19444 better deprecation message

comment:13 Changed 5 years ago by chapoton

  • Status changed from needs_work to needs_review

Done

comment:14 Changed 5 years ago by jdemeyer

This is wrong:

-        k = int(ceil(log(n,2)))
+        k = int(ceil(RDF(n).log(2)))

Do not use floating-point computations when you want an exact answer.

An exception to this would be MPFR (used in Sage by RR) because that does give a guarantee of exactness if possible.

So, you should either use ZZ or RR to do this computation.

comment:15 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:16 Changed 5 years ago by jdemeyer

I would replace ceil(log(n,2)) by (ZZ(n) - 1).nbits() which is guaranteed to be correct and reasonably fast.

comment:17 Changed 5 years ago by git

  • Commit changed from 465125e6133b5f33493d1f78b98237228bb7dae4 to e09327389baa0b96060ac5ea36cf77b69ee6dab6

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

59f1ee4trac 19444 using nbits
e093273Merge branch 'u/chapoton/19444'

comment:18 Changed 5 years ago by chapoton

ok ; but now there remains an RDF in "src/sage/rings/finite_rings/element_ntl_gf2e.pyx"

comment:19 Changed 5 years ago by git

  • Commit changed from e09327389baa0b96060ac5ea36cf77b69ee6dab6 to 5489453100b096923d1376b82139c4d3952edd83

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

ed4e22aMerge branch 'u/chapoton/19444' in 8.1.b4
5489453trac 19444 do not use RDF().log()

comment:20 Changed 5 years ago by chapoton

  • Status changed from needs_work to needs_review

should be good now

comment:21 Changed 5 years ago by jdemeyer

In the light of __future__ division, it is better to use // 8 instead of / 8.

comment:22 Changed 5 years ago by git

  • Commit changed from 5489453100b096923d1376b82139c4d3952edd83 to d2f7962a6ee8a2e6e0b174083a44b4ed067b5785

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

d2f7962trac 19444 double //

comment:23 Changed 5 years ago by chapoton

done

comment:24 Changed 5 years ago by chapoton

bot is morally green

comment:25 Changed 5 years ago by slabbe

  • Status changed from needs_review to positive_review

comment:26 Changed 5 years ago by vbraun

  • Branch changed from u/chapoton/19444 to d2f7962a6ee8a2e6e0b174083a44b4ed067b5785
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.