Skip to content

added sumOfLog2s#2500

Merged
quickfur merged 1 commit intodlang:masterfrom
9il:sum-of-logs
Sep 23, 2014
Merged

added sumOfLog2s#2500
quickfur merged 1 commit intodlang:masterfrom
9il:sum-of-logs

Conversation

@9il
Copy link
Member

@9il 9il commented Sep 9, 2014

Computes accurate sum of binary logarithms of input range.

@DmitryOlshansky
Copy link
Member

So I see this just uses the formula - logarithm of products is a sum of logarithms to speed up the latter.
Can't really comment on FP precision checking, but otherwise the idea is nice.

std/numeric.d Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ElementType!Range needs to be constrained

@9il
Copy link
Member Author

9il commented Sep 10, 2014

@DmitryOlshansky, accuracy like log2(r1...rn), but wihtout exponent overflow

@9il
Copy link
Member Author

9il commented Sep 10, 2014

With native frexp (PR #2508) it is 4.5 times faster then sum of logs when compiled with LDC.

@9il 9il changed the title added sum of logs to std.numeric std.numeric: added sumOfLogarithms Sep 11, 2014
@9il 9il changed the title std.numeric: added sumOfLogarithms added sumOfLogs Sep 17, 2014
@9il
Copy link
Member Author

9il commented Sep 17, 2014

Rebased

std/numeric.d Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the new return type is an improvement, ElementType!Range still needs to be constrained. For example if(isInputRange!Range && isNumeric!(ElementType!Range)) (though by the looks of it, it won't work well with integers?).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JakobOvrum A have added isFloatingPoint!(ElementType!Range). It looks like all std.numeric without checks like isFloatingPoint. For example

ElementType!Range entropy(Range)(Range r) if (isInputRange!Range)
{
    Unqual!(typeof(return)) result = 0.0;
    foreach (e; r)
    {
        if (!e) continue;
        result -= e * log2(e);
    }
    return result;
}

@quickfur
Copy link
Member

LGTM

@dnadlinger
Copy link
Contributor

Shouldn't this be sumOfLog2s or something for consistency?

@9il
Copy link
Member Author

9il commented Sep 23, 2014

All std.numeric uses only log2. But names without 2. I can rename if you ask to do this.

@dnadlinger
Copy link
Contributor

@9il: I don't think std.numeric has any other functions with "log" in the name, does it? The only mentions in 2.066 seem to be in some comments, where the base is explicitly specified. std.math uses log2, hence the question.

@9il
Copy link
Member Author

9il commented Sep 23, 2014

@klickverbot Yes ) I will change it =)

@9il 9il changed the title added sumOfLogs added sumOfLog2s Sep 23, 2014
@9il
Copy link
Member Author

9il commented Sep 23, 2014

Rebased

std/numeric.d Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to rename these calls. The autotester will probably report failure. :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

renamed, and updated

remove CustomFloat support

remove blank line

update comment

update return type

rename sumOfLogs -> sumOfLog2s

update unittest
@9il
Copy link
Member Author

9il commented Sep 23, 2014

Rebased again)

@quickfur
Copy link
Member

LGTM

@quickfur
Copy link
Member

Auto-merge toggled on

quickfur pushed a commit that referenced this pull request Sep 23, 2014
@quickfur quickfur merged commit e4e2739 into dlang:master Sep 23, 2014
@9il 9il deleted the sum-of-logs branch September 23, 2014 20:03
@9il 9il mentioned this pull request Apr 28, 2016
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.

5 participants