Add findLocalMin

[9il]

It is fixed version of R's optimise.

fBlocked by https://issues.dlang.org/show_bug.cgi?id=15204

[kyllingstad]

Is this implementation based on R's source code? If so, it probably cannot be included in Phobos, as R is licensed under GPL.

[9il]

@kyllingstad R's implementation is translation from Brent's book (source code is in book!). I have checked each line and rewrite algorithm from scratch. Both R and book's algorithms look identical and have the same bug (infinity loop), that was fixed in this PR.

[9il]

... "look identical" includes cycle structures and variable names.

[DmitryOlshansky]

This is cute) I think we had at least one Brent's algorithm in std.numeric ?

[9il]

This is cute) I think we had at least one Brent's algorithm in std.numeric ?

std.numeric.findRoot is a modification of Brent's algorithm.

[kyllingstad]

R's implementation is translation from Brent's book (source code is in book!).

Ok, then this should be fine. I just had to make sure. (D has had its share of licence issues in the past, and we don't want to get into that mess again.)

[kyllingstad]

I think assert messages should describe the error, not the expected state. E.g., this could be:

assert(isFinite(ax) && isFinite(bx), "ax and/or bx are not finite");

But this is a matter of taste, I guess. I don't know what is the Phobos convention (if there is any) here. Also, this should be two asserts, so the user immediately knows which argument is bad.

[kyllingstad]

Two general comments:

• You should add tests for all floating-point types, not just double.
• There are a bit too many empty lines in here for my taste, both inside the function and between some of the unittests. The whole thing could be a lot more compact without affecting readability.

[9il]

You should add tests for all floating-point types, not just double.
There are a bit too many empty lines in here for my taste, both inside the function and between some of the unittests. The whole thing could be a lot more compact without affecting readability.

done

[kyllingstad]

My comment about assert messages holds for the other two as well.

[kyllingstad]

Well, I don't have any further comments. I guess some of the more active Phobos devs should comment on any remaining style issues, and whether this functionality is general enough to warrant inclusion in Phobos. (I certainly think it is!)

[9il]

Thanks!

On 18 Aug 2015, at 11:19, Lars T. Kyllingstad notifications@github.com wrote:

Well, I don't have any further comments. I guess some of the more active Phobos devs should comment on any remaining style issues, and whether this functionality is general enough to warrant inclusion in Phobos. (I certainly think it is!)

—
Reply to this email directly or view it on GitHub #3559 (comment).

[9il]

Fails because magic in 32 bit release mode:

writeln("ttt ", [i, a, b, m, (a + b) / 2, (a + b) / 2.0, ], a <= m, m <= b);

produce:

ttt [8, -5.50533e+307, 4.49423e+307, -8.08875e+307, -8.08875e+307, -8.08875e+307]truetrue

O_O So, (-5.50533e+307 + 4.49423e+307) / 2.0 = -8.08875e+307 :-(

[quickfur]

Hmm.

static assert(approxEqual((2*c - 3)^^2, 5.0);

? Probably total overkill, but I get nervous with these hexadecimal float constants obtained from external sources that basically have to be taken on faith.

[quickfur]

Also, please squash commits, or at least put readable commit messages for them. :-)

[9il]

Also, please squash commits, or at least put readable commit messages for them. :-)

Please read my last comment about 32 bit release mode bug. I have no idea what I can do with this.

[DmitryOlshansky]

Oops, wrong button.

Fails because magic in 32 bit release mode:

File bug report, it looks like codegen bug. Have you tried LDC/GDC?

[9il]

I will try with LDC and file it.

[9il]

OSX works fine both with LDC and DMD. But FreeBSD/Linux fails with DMD. ISSUE https://issues.dlang.org/show_bug.cgi?id=15204

[DmitryOlshansky]

FreeBSD32 passed recently

[9il]

Cleaned and rebased

[DmitryOlshansky]

As this is a name addition so CC @andralex

[andralex]

The range notation does not exist in D or in math. Gotta choose one :o).

[9il]

I just follow findRoot naming: Given a function f and a range [a..b] ...

[andralex]

I approve the name addition, please mind the nits before pulling. Thx!

[9il]

#3559 (findLocalMin), #2500 (sumOfLog2s), and findRoot fixes and optimizations: #3028, #2639, #2524, #2266

[9il]

I didn't squash commit because the last contains style fixes for all std.numeric.
Ready to merge

[DmitryOlshansky]

Auto-merge toggled on

[9il]

just removed the link to wikipedia, because this refers to root-finding algorithm instead of optimization algorithm

[wilzbach]

just removed the link to wikipedia, because this refers to root-finding algorithm instead of optimization algorithm

You are lucky that @braddr hasn't forbidden/fixed this yet braddr/d-tester#49 ;-)