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

Make log also accept a base. #1667

Merged
merged 6 commits into from
Apr 15, 2017
Merged

Make log also accept a base. #1667

merged 6 commits into from
Apr 15, 2017

Conversation

LeifAndersen
Copy link
Member

(log a b) = (/ (log a) (log b)), however it has the potential to run faster.

@mflatt
Copy link
Member

mflatt commented Apr 12, 2017

The base shouldn't be 1, and I think you want an error from log in that case, instead of an error from /. A base of 0 happens to provoke an errr from log, but you may want a more direct check.

@LeifAndersen
Copy link
Member Author

Ah, good catch. I will add error checking for that. Thanks.

@soegaard
Copy link
Member

FWIW for testing fllogb from math/flonum can be used.

[http://docs.racket-lang.org/math/flonum.html?q=log#%28def._%28%28lib._math%2Fflonum..rkt%29._fllogb%29%29](fllogb in math/flonum)

@LeifAndersen
Copy link
Member Author

@soegaard Interesting. Although that function only works with flonums, while the log function only works for fixnums.

Either way, thanks. For the life of me I couldn't find an arbitrary log base function in all of our stdlib.

Does anyone else have any thoughts?

@LeifAndersen
Copy link
Member Author

Woops, I mean, the log function (in racket/base), also works on fixnums.

@LeifAndersen
Copy link
Member Author

Okay, @mflatt and @soegaard , I think its ready to merge. The tests are failing, but that seems to be because they are currently failing in master as well. Thoughts?

@soegaard
Copy link
Member

soegaard commented Apr 14, 2017 via email

@LeifAndersen
Copy link
Member Author

@soegaard @mflatt

Perhaps recommend using fllogb in case where accuracy is important?

Good call. I'll update the docs to reflect that tonight.

Somehow it feels odd to have "almost" duplicated functionality. Should log use the same algorithm as fllogb ?

I don't care one way or another. To me, it makes sense to have (log a b) defined as (/ (log a) (log b)), but has the potential to be computed faster. But if we want to use the same algorithm as fllogb, I guess that's fine too. It just seems odd to me to have:
(log x) and (log x (exp 1)) return slightly different values because they are using a different algorithm. (The existing log algorithm in the first case and the fllogb algorithm in the second case.)

Thoughts?

One other thing, does anyone have a preference of whether this gets put into 6.9, or should wait until 6.10? I'm okay either way, but if we want 6.9 I should merge this tonight.

@LeifAndersen
Copy link
Member Author

Since it seems like everyone is okay I'm going to go ahead and merge this.

@LeifAndersen LeifAndersen merged commit 922b5f0 into racket:master Apr 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants