Closed Titaniumtown closed 2 years ago
Thanks for the pull request!
log
. (https://www.wolframalpha.com/input?i=exp%28log%28x%29%29)log
to ln
for the natural logarithm. But log
should not refer to log10
. log
would then not be available anymore.partial/mod.rs
according to the change, here and here.1. My undergraduate analysis professor had and Wolfram Alpha has a different opinion about the default meaning of `log`. (https://www.wolframalpha.com/input?i=exp%28log%28x%29%29)
Ah lol. That's not what I was taught lol. But if that's the way of doing it 🤷🏻
2. However, in order to avoid confusion I am fine with renaming `log` to `ln` for the natural logarithm. But `log` should not refer to `log10`. `log` would then not be available anymore.
Sounds good!
3. You have to adapt `partial/mod.rs` according to the change, [here](https://github.com/bertiqwerty/exmex/blob/cb73fa8481edaecb0c604928c4b362e8388b4618/src/partial/mod.rs#L811) and [here](https://github.com/bertiqwerty/exmex/blob/cb73fa8481edaecb0c604928c4b362e8388b4618/src/partial/mod.rs#L943).
Got it.
I think what I pushed addresses everything 👍🏻
Almost there, I think. As indicated by the compilation error, you have to add another line
unary_name!(log10, Float);
for log10
after
https://github.com/bertiqwerty/exmex/blob/cb73fa8481edaecb0c604928c4b362e8388b4618/src/value.rs#L334
And some documentation needs to be changed. https://github.com/bertiqwerty/exmex/blob/9c5d5e4edbc8a6b302eb67874442a4fbaecb56ae/src/lib.rs#L20
As indicated by the compilation error
I didn't get any compilation error. but I implemented the changes you mentioned.
Can you run
cargo test --all-features
on your side and fix the errors? I think, some test-strings need to be adapted. See https://github.com/bertiqwerty/exmex/runs/5496830706?check_suite_focus=true
cargo test --all-features
Oh yes. My bad.
Just something I noticed: derivatives of log2
and log10
don't seem to work. the derivative of log of x
with base n
should be equal to 1/(x*ln(n))
.
Ah. Yes. Right. They are not implemented. Let's first merge this pull request. I will add them later. Shouldn't be a big thing.
Perfect. Checks have passed. Thanks for your contribution.
I have added the missing derivatives. Commit ad082c9759304649bd8c5ccf00ac110878106a09.
Thank you!
Usually
log
refers to taking the logarithm of a function with base 10. But currently this returns the logarithm with basee
(e being Euler's number).This PR fixes that issue by making
log
refer tolog10
, andln
referring to the actual functionln
refer to logarithm with basee
.I believe I did this properly. but do let me know!