Skip to content

Feature/log2 - #337

Closed
marc-mabe wants to merge 4 commits into
php:masterfrom
marc-mabe:feature/log2
Closed

marc-mabe wants to merge 4 commits into
php:masterfrom
marc-mabe:feature/log2

Conversation

@marc-mabe

Copy link
Copy Markdown
Contributor

This PR adds the function log2. It works the same as the C function and is an addition to the always translated C functions to PHP like log1p and log10

It also fixes an issue on 32bit environments calculating the second logarithm as an integer:

var_dump(log2(64), (int)log2(64)); // float(6) & int(6)
var_dump(log(64, 2), (int)log(64, 2)); // float(6) & int(5)

The method log2 is part of the C99 standard - so there is no HAVE_LOG2 check done.
(Not tested if some environments doesn't support it)

@yohgaki

yohgaki commented Jul 18, 2013

Copy link
Copy Markdown
Contributor

Having basic math functions is good. IMO.
I think configure test is not required for log2().
Comments?

@dsp

dsp commented Sep 16, 2013

Copy link
Copy Markdown
Member

Thanks for this pull request. We usually try to avoid adding new global functions to the core. We wan't to avoid clashes with existing functions and avoid maintenance overhead. In this case there is a log(X, 2) equivalent to log2 so it would be just an abbrev that saves you 3 chars, which is not sufficient to have possible BC breaks in existing code bases. Imagine people using:

function log2($x) {
   return log($x, 2);
}

in their code. The new function would cause a fatal error for close to no gain. Therefore I am closing this PR.

@dsp dsp closed this Sep 16, 2013
@marc-mabe

Copy link
Copy Markdown
Contributor Author

@dsp Dit you read the example of the logarithm of 2 issue (not bug) on 32bit platforms using the method log()

var_dump(log2(64), (int)log2(64)); // float(6) & int(6)
var_dump(log(64, 2), (int)log(64, 2)); // float(6) & int(5)

That's the reason I added it - not saving of 2 characters.

@dsp

dsp commented Sep 16, 2013

Copy link
Copy Markdown
Member

@marc-mabe The example is a rounding error in floating point calculation. It's a standard problem when casting floats to int, therefore you cannot rely on the conversation (int)x==(float)y. Therefore I don't think this is a bug, but rather just happens due to how floating point works. Also if it one might consider it being a bug, we want to start fixing it in log(). For example check if the second argument in log() is 2 and use log2() internally instead to be less prone to floating point issues.

@marc-mabe

Copy link
Copy Markdown
Contributor Author

@dsp As noted above it's not a bug, it's an issue / problem. Sure it's coming from rounding errors but log2 have less errors.

Calling log2 internally on log(?, 2) is good for me. Should I create a new PR or can you reopen this one?

Also for consistency I think the already implemented methods log1p and log10 should than be marked as deprecated and it's very simple to be done on log

@dsp

dsp commented Sep 16, 2013

Copy link
Copy Markdown
Member

Please create a new PR. Also ensure that we check if log2 is available on the system and ifdef the code if necessary. PHP relys on C89 and not C99 and has to work with multiple compilers like ICC/SunCC, etc on different platforms (including Solaris, AIX, etc). Not all of them might have a C99 compatible libc.

@nikic

nikic commented Sep 16, 2013

Copy link
Copy Markdown
Member

@marc-mabe log1p can't be implemented using log because it has different accuracy requirements. Same for expm1 and exp.

@marc-mabe

Copy link
Copy Markdown
Contributor Author

@nikic you are right - but log[2|10](x) is the same as log(x, [2|10]) with this patch (https://github.com/marc-mabe/php-src/blob/feature/log_improvement/ext/standard/math.c#L697)

So log10 can be deprecated. Does this need an RFC?

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.

4 participants