besselj Float32 calling incorrect function #15272
Conversation
|
Aha. Strange that this would be intermittent? |
|
Yeah, very odd. I don't know enough about the ABI, but I guess in certain cases we happened across the right bit pattern. |
|
Looks like we only have very limitted test for |
simonbyrne
force-pushed
the
sb/besselfix
branch
from
faa1c83 to
9cb2b2a
Compare
|
I've added some more tests. |
simonbyrne
force-pushed
the
sb/besselfix
branch
2 times, most recently
from
4017ebe to
63da79b
Compare
| @@ -387,12 +387,20 @@ j33 = besselj(3,3.) | |||
| @test besselj(-3,-3) == j33 | |||
| @test besselj(-3,3) == -j33 | |||
| @test besselj(3,-3) == -j33 | |||
| @test besselj(3,3f0) ≈ j33 | |||
There was a problem hiding this comment.
do these have to be approximate because we can't reliably constrain different libm's accuracy here?
There was a problem hiding this comment.
No, it's because they're calling different precision functions, so they won't be exactly equal.
There was a problem hiding this comment.
but if we round the higher-precision value into the lower precision type can we reliably test exact equality?
There was a problem hiding this comment.
In that case, it's up to the libm/openspecfun (there are 3 different C functions involved)
There was a problem hiding this comment.
okay so if the answer is no even to the rounded version, then not worth worrying about
There was a problem hiding this comment.
and maybe it needs an even larger tolerance?
There was a problem hiding this comment.
Or maybe cast the RHS to Float32.
julia> 0.1320342f0 ≈ 0.1320341839246122
false
julia> 0.1320342f0 ≈ Float32(0.1320341839246122)
trueThere was a problem hiding this comment.
Hmm, perhaps we should change isapprox to use the larger of the 2 tolerances.
There was a problem hiding this comment.
That would make more sense. Probably a different issue/pr though...
There was a problem hiding this comment.
Let's do Float32 on the rhs now, and think about tweaking isapprox separately
|
I think I broke travis by doing a bunch of force pushes, and so the most recent commit doesn't appear. Any way I can get it to restart? |
|
f0b8cc8 is merging the right commit, someone just clicked cancel... |
|
Isn't it 63da79b? |
|
ah, makes sense. I've restarted. |
simonbyrne
force-pushed
the
sb/besselfix
branch
from
63da79b to
38f988d
Compare
|
Okay, I've updated the |
|
great, do merge when green |
besselj Float32 calling incorrect function
Hopefully should fix recent 32-bit build failures (e.g. #15259).