promote_op/broadcast should not call the function for one(T)

[stevengj]

As was commented at #17300 (comment) and #17172 (comment), the promote_op function has a fallback method that calls typeof(op(one(T))). This is not really acceptable.

For one thing, op(1) may not even be valid—it may throw a DomainError e.g. if op(x)=log(log(log(x))). Even if 1 is a valid argument, it is unexpected and undesirable that op should be called without the user requesting it. (e.g. imagine if op has side effects, or is simply an extremely expensive function.)

[stevengj]

cc @nalimilan, @JeffBezanson

[andreasnoack]

Maybe the guideline should be not to broadcast functions with side effects. As mentioned in #17300, using a version of log that doesn't throw completely fixes the issue here. It might be too hard to find that version of log though and we throw a lot in Base.

I'm not really concerned about the cost of a single extra call to a function you already want to broadcast. To be significant, it would require that the function is very expensive and it's broadcast over a very short range. I'm not sure that case should be given too much weight in this decision.

[stevengj]

@andreasnoack, it totally does not fix the issue to change log. log is just an example. "Don't pass functions that throw exceptions on 1" is a bizarre and counter-intuitive rule for broadcast.

Nor do I think it is sensible to forbid side-effects, or assume that function calls are cheap — it is quite common to have functions f that perform some expensive computation (often printing a status message or writing output to a file as a side effect), and to broadcast/map them over a small array in order to do a parameter sweep.

In my mind, it is unacceptable for a programming language to call my function for arguments that I don't supply myself.

Can't we just get rid of the fallback entirely, and use the map/comprehension algorithm in that case? Isn't that the whole point of #17172?

[nalimilan]

IIUC Jeff's concern is that the map/broadcast algorithm for guessing the type could hurt performance, in particular by preventing SIMD: #17172 (comment) To make progress on that front, we'd need to check whether that's the case or not.

[nalimilan]

I also wonder whether whether inference couldn't allow for a fast-path when the called function is type-stable for the passed input types.

[JeffBezanson]

Inference already works fine here. If we can infer the result type of the function the code is specialized down to a compact loop. Anyway I think I should just merge #16622, which provides return_type.

[nalimilan]

So in theory there should be no problem with removing that method?

[stevengj]

Can this be fixed now that #16622 was merged?

[pabloferz]

@stevengj See #17389

[ssfrr]

This also causes a problem in SIUnits.jl: Keno/SIUnits.jl#84.

In the issue above the error is because unit-less numbers can't be converted to unitful numbers, so it fails trying one(typeof(Hz))

Also if you try collect(1:5) * (1Hz) you get a conversion error on setindex. This is because one(typeof(1Hz)) is 1 (because the multiplicative identity of a unitful number doesn't have any units).

So this is a case where one(T) isn't a good representative value to determine the eltype for the result array.

[stevengj]

As @yuyichao pointed out on julia-users, this also causes a MethodError for e.g. broadcast(muladd,[1],[2],[3]), because the promote_op fallback that passes one(T) assumes that every function accepts one or two arguments.

Marking the issue as a regression since these problems weren't present in 0.4.

[ssfrr]

Seems that the commit that closed this issue has been reverted (dbdaf72). Should the issue be re-opened?

[tkelman]

it was reverted on release-0.5. if it can be fixed in a way that doesn't hurt performance and doesn't break packages, we can consider re-applying along with backporting the fix