Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use native log10 for logBase with base of 10 #1054

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

richyliu
Copy link

Current logBase implementation is not precise enough. For example: logBase 10 100 == 2 but logBase 10 1000 /= 3.

The proposed change is to use a separate native Javascript log10 function for logs with base 10.

This would close #1048

@gampleman
Copy link

Would it make sense to special case also Math.log2 and Math.log1p?

@rupertlssmith
Copy link

rupertlssmith commented Feb 18, 2022

This seems to fail to compile, due to using the == operator in Base.elm, where it is defined:

> elm make
Detected problems in 1 module.
-- UNKNOWN OPERATOR --------------------------------------------- src/Basics.elm

I do not recognize the (==) operator.

621|   if base == 10 then
               ^^
Is there an `import` and `exposing` entry for it?

It needs to be done like this:

if eq base 10 then
@rupertlssmith
Copy link

I would like to add this PR to elm-janitor: https://github.com/elm-janitor/manifesto/blob/master/README.md

Would you be able to fix the compile error to get this patch over the line?

@rupertlssmith
Copy link

I have another suggestion, which is to extend the test over a larger range of powers of 10. This will test the entire range of a double precision float:

        log10Tests =
          describe "Check accuracy of log base 10"
            (List.map
              (\i -> test ("logBase 10 " ++ (10^i |> toString)) <|
                \() -> Expect.equal (toFloat i) (logBase 10 (10^(toFloat i)))
              )
              (List.range -308 308)
            )
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants