Julia port of Openlibm expm1. #25903

pkofod · 2018-02-05T22:17:03Z

This works, but is a bit slower than the C version, and I'm not quite sure why.

I put the definitions in a module JuLibm, opened julia with -O3 punched in

for x in [0.0,0.3,0.9,4.0,20.0,120.0]
       xf0 = Float32(x)
       println("\n --- Benchmarking $x")
       display(@benchmark JuLibm.expm1($x))
       display(@benchmark expm1($x))
       println("\n --- Benchmarking $(xf0)")
       display(@benchmark JuLibm.expm1($xf0))
       display(@benchmark expm1($xf0))
end

and got
~~https://gist.github.com/pkofod/c4dda880815985e6020ea58730d315cd~~ https://gist.github.com/pkofod/d666951a7886f9f75ad013782b61acd9

there will be tests, but I'm just posting this to get some feedback about what to look for. I have a feeling the problem here is the same one that is causing regressions here #24859

Design suggestions and comment suggestions are also very welcome!

tests
license info update
fix performance

and then we're golden

ararslan · 2018-02-05T22:55:33Z

base/special/expm1.jl

+
+    # filter out huge and non-finite argument
+	if absx >= expm1_huge(T) # quit early for large absolute values of x
+        if isinf(absx)


The indentation in this file is kind of all over the place. Should be 4-space indents everywhere.

I will fix it. Not sure why that happened though...

simonbyrne · 2018-02-05T23:42:48Z

I rebuilt with these functions, and am benchmarking against 0.6, but not seeing any difference.

pkofod · 2018-02-06T06:36:30Z

You're getting comparable timings?

Edit: oh, I seem to be getting the exact same timings as well. Great, then I just need to polish this.

stevengj · 2018-02-06T17:25:48Z

base/special/expm1.jl

+    # filter out huge and non-finite argument
+	if absx >= expm1_huge(T) # quit early for large absolute values of x
+        if isinf(absx)
+            return xsign == true ? x : -T(1.0)


Probably just xsign, not xsign == true

stevengj · 2018-02-06T17:29:58Z

base/special/expm1.jl

+            end
+	    else
+            #            k  = round(Int32, expm1_invln2(T)*x)
+            k = unsafe_trunc(Int32, expm1_invln2(T)*x + (!xsign ? 0.5 : -0.5))


possibly ifelse(xsign, -0.5, 0.5)

Also, T(0.5)?

to avoid the branch? and yes, T(0.5) to be safe!

ararslan · 2018-02-12T19:58:17Z

@nanosoldier runbenchmarks(ALL, vs=":master")

stevengj · 2018-02-12T22:12:41Z

base/special/expm1.jl

+            # the next line could be replaced with the above but we don't need the
+            # safety of round, so we round by adding one half with the appropriate
+            # sign and truncate to Int32
+            k = unsafe_trunc(Int32, expm1_invln2(T)*x + ifelse(xsign, -T(0.5), 0.5))


T(0.5) for the other branch too.

stevengj · 2018-02-12T22:13:57Z

base/special/expm1.jl

+        if k == -1
+            return T(0.5)*(x - e) - T(0.5)
+        end
+        if k ==1


missing space

stevengj · 2018-02-12T22:16:34Z

base/special/expm1.jl

+                return  T(1.0) + T(2.0)*(x - e)
+            end
+        end
+        two_pow_k = expm1_two_pow_k(T, k) # T(2.0)^k but 9-10 times faster


Should this be merged into exp2? Maybe we should have an exp2(T, x::Integer) method.

nanosoldier · 2018-02-13T01:04:37Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

pkofod · 2018-02-14T22:16:00Z

can nansoldier run here again now that the benchmarks are tuned? @ararslan

ararslan · 2018-02-14T22:18:31Z

Yep, I recommend fixing the branch conflicts first though.

ararslan · 2018-02-14T22:22:23Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2018-02-15T03:29:24Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

pkofod · 2018-02-20T15:39:54Z

CI seems unrelated as usual (it was a few sweet weeks of green CI we had :))

pkofod · 2018-05-16T12:16:56Z

Nanosoldier can be activated again here.

KristofferC · 2018-05-16T12:18:25Z

@nanosoldier runbenchmarks("scalar", vs=":master")

nanosoldier · 2018-05-16T14:15:17Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

pkofod · 2018-05-16T15:19:30Z

A few expm1 "regressions", but they might be flukes. Though, they're specific to Float32, so I'll have a look locally.

ViralBShah · 2018-05-16T20:40:24Z

Also, the new optimizer is enabled now.

pkofod · 2018-05-17T12:05:06Z

Comments for the PR in general or nanosoldier specifically ? @simonbyrne @stevengj

ViralBShah · 2018-05-22T12:59:28Z

It's likely because of the new optimizer - @Keno

Probably shouldn't hold merging if all else is good, and we can have a new performance issue then.

pkofod · 2018-05-22T17:00:34Z

Can I turn the new optimizer off locally? Or is it no longer a flag on master?

Keno · 2018-05-22T17:09:18Z

The flag should still work enable_new_optimizer in base/optimize.jl

pkofod · 2018-05-22T17:36:53Z

The flag should still work enable_new_optimizer in base/optimize.jl

thanks, I'll try it without to see if that's why it regresses.

pkofod · 2018-06-07T09:02:54Z

I think this is good to go, unless @simonbyrne has some rewrites (though they could always be done later as well)

stevengj · 2018-06-09T15:18:36Z

base/special/expm1.jl

@@ -0,0 +1,238 @@
+# ====================================================


Probably the comment should start with:

# This code is a Julia translation of the C code from ... with the following license:

to make it clear that this is not the original Sun code.

Also, see https://github.com/JuliaLang/julia/blob/master/base/special/exp.jl

ViralBShah · 2018-07-08T15:21:12Z

Bump.

ararslan · 2018-07-08T20:11:56Z

Rebased on master.

pkofod · 2018-07-17T20:02:32Z

I can change the license comment as suggested by sgj when I get back from vacation, but if someone can push a commit with the changes before that, feel free.

pkofod · 2018-07-20T12:48:49Z

Fixed the license comment. Should we run nanosoldier here as well?

KristofferC · 2018-07-20T12:49:36Z

@nanosoldier runbenchmarks("scalar", vs=":master")

ararslan · 2018-07-20T19:47:07Z

Nanosoldier is being weird. @nanosoldier runbenchmarks("scalar", vs=":master")

nanosoldier · 2018-07-20T21:43:48Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

pkofod · 2018-07-22T11:14:02Z

@simonbyrne thoughts on these benchmarks?

StefanKarpinski · 2018-07-22T18:05:01Z

base/math.jl

@@ -277,7 +277,8 @@ asinh(x::Number)
 Accurately compute ``e^x-1``.
 """
 expm1(x)
-for f in (:exp2, :expm1)
+
+for f in (:exp2,)


Now that this is down to a single function, it would make sense to get rid of the loop.

Yes, I should have made a comment after I did this. The only reason why I did it like this is because it was easier for me, and the remaining code will be removed when I rebase #24859 after this is merged. Both should be quite close to merge-able I think.

I can remove the loop in this PR if you prefer it for a "clean" git history.

(on a side note, having removed these functions from the tuple one by one, I'm very happy to see the loop gone soon :))

stevengj · 2018-07-23T18:53:26Z

base/special/expm1.jl

@@ -1,3 +1,4 @@
+# This code is a Julia translation of the C code from ... with the following license:


the ... should be replaced with the actual source(s) of the C code

Ah, I actually saw that, but I was pretty sure I changed it. Lol

Just double checked , we’re you commenting on an old commit?

oscardssmith · 2021-04-03T04:57:51Z

Superseded by #37440

ararslan requested review from simonbyrne and stevengj February 5, 2018 22:54

ararslan added the maths Mathematical functions label Feb 5, 2018

ararslan reviewed Feb 5, 2018

View reviewed changes

ararslan added the needs tests Unit tests are required for this change label Feb 5, 2018

stevengj reviewed Feb 6, 2018

View reviewed changes

pkofod force-pushed the expm1-2 branch from b2c58c8 to a2d8728 Compare February 6, 2018 22:22

ararslan removed the needs tests Unit tests are required for this change label Feb 10, 2018

pkofod mentioned this pull request Feb 11, 2018

exp2, expm1, cbrt benchmarks. JuliaCI/BaseBenchmarks.jl#185

Merged

stevengj reviewed Feb 12, 2018

View reviewed changes

base/special/expm1.jl Outdated

if k == -1

return T(0.5)*(x - e) - T(0.5)

end

if k ==1

Copy link

Member

stevengj Feb 12, 2018

Choose a reason for hiding this comment

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

missing space

stevengj reviewed Feb 12, 2018

View reviewed changes

simonbyrne mentioned this pull request Mar 13, 2018

Remove openlibm #26434

Open

17 tasks

pkofod force-pushed the expm1-2 branch from 591516c to 12a8cf0 Compare May 15, 2018 19:36

stevengj reviewed Jun 9, 2018

View reviewed changes

pkofod added 6 commits July 8, 2018 13:08

Julia port of Openlibm expm1. [skip ci]

cd44a1f

Simplify condition and fix bug for Inf inputs.

c9d6e35

Fix small things and add some tests based on exp tests.

765b17d

Fix truncation.

501dbeb

Fix license.

3d36f1f

Add T() and space around an ==.

990f6a8

ararslan force-pushed the expm1-2 branch from df1b960 to 990f6a8 Compare July 8, 2018 20:11

fix license comment

3554df4

more license comment editing

8955dcc

Merge branch 'master' into expm1-2

f2d81e7

pkofod changed the title ~~[WIP] Julia port of Openlibm expm1.~~ Julia port of Openlibm expm1. Jul 22, 2018

StefanKarpinski reviewed Jul 22, 2018

View reviewed changes

remove loop over one element tuple

8c3a813

stevengj reviewed Jul 23, 2018

View reviewed changes

oscardssmith closed this Apr 3, 2021

		@@ -0,0 +1,238 @@
		# ====================================================

		@@ -1,3 +1,4 @@
		# This code is a Julia translation of the C code from ... with the following license:

Julia port of Openlibm expm1. #25903

Julia port of Openlibm expm1. #25903

Conversation

pkofod commented Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbyrne commented Feb 5, 2018

pkofod commented Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanosoldier commented Feb 13, 2018

pkofod commented Feb 14, 2018

ararslan commented Feb 14, 2018

ararslan commented Feb 14, 2018

nanosoldier commented Feb 15, 2018

pkofod commented Feb 20, 2018

pkofod commented May 16, 2018

KristofferC commented May 16, 2018

nanosoldier commented May 16, 2018

pkofod commented May 16, 2018

ViralBShah commented May 16, 2018

pkofod commented May 17, 2018

ViralBShah commented May 22, 2018

pkofod commented May 22, 2018

Keno commented May 22, 2018

pkofod commented May 22, 2018

pkofod commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViralBShah commented Jul 8, 2018

ararslan commented Jul 8, 2018

pkofod commented Jul 17, 2018

pkofod commented Jul 20, 2018

KristofferC commented Jul 20, 2018

ararslan commented Jul 20, 2018

nanosoldier commented Jul 20, 2018

pkofod commented Jul 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscardssmith commented Apr 3, 2021

pkofod commented Feb 5, 2018 •

edited

Loading

pkofod commented Feb 6, 2018 •

edited

Loading