Weird behavior with fixed point colorant, floating point colorant, and ifelse

[galenlynch]

I'm running into some weird behavior that I'm at a loss to explain.

I want to change the color of some pixels in an image, and discovered some very strange corner cases when assigning type unstable values to an array of colorants.

I would expect the following to produce an array filled with RGB{N0f8}(1.0, 1.0, 1.0), but instead it produces RGB{N0f8}(1.0, 0.0, 0.0):

julia> using Images

julia> input = Matrix{RGB{N0f8}}(undef, 5, 5);

julia> fill!(input, RGB(0, 0, 0));

julia> useother = trues(size(input));

julia> function transfermask(input, useother, othervalue)
           combined = similar(input)
           for idx in eachindex(combined)
               combined[idx] = ifelse(
                   useother[idx],
                   RGB{Float64}(othervalue),
                   input[idx]
               )
           end
           return combined
       end
transfermask (generic function with 1 method)


julia> transfermask(input, useother, RGB{N0f8}(1.0,1.0,1.0))
5×5 Array{RGB{N0f8},2} with eltype RGB{Normed{UInt8,8}}:
 RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)
 RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)
 RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)
 RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)
 RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)

However, the following both work, even though they seem functionally identical to me:

julia> function ok_transfermask(input, useother, othervalue)
           combined = similar(input)
           for idx in eachindex(combined)
               combined[idx] = ifelse(
                   useother[idx],
                   RGB{Float64}(RGB{N0f8}(1, 1, 1)),
                   input[idx]
               )
           end
           return combined
       end
ok_transfermask (generic function with 1 method)

julia> function also_ok_transfermask(input, useother, othervalue)
           combined = similar(input)
           for idx in eachindex(combined)
               combined[idx] = RGB{Float64}(othervalue)
           end
           combined
       end
also_ok_transfermask (generic function with 1 method)

julia> ok_transfermask(input, useother, RGB{N0f8}(1.0,1.0,1.0))
5×5 Array{RGB{N0f8},2} with eltype RGB{Normed{UInt8,8}}:
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)

julia> also_ok_transfermask(input, useother, RGB{N0f8}(1.0,1.0,1.0))
5×5 Array{RGB{N0f8},2} with eltype RGB{Normed{UInt8,8}}:
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)

These results don't make any sense to me.

[johnnychen94]

It's weird to me as well. As a supplement, if you use the Debugger to trace the result, it works normally inside of transfermask:

1|debug> n
In transfermask(input, useother, othervalue) at REPL[5]:2
  6              RGB{Float64}(othervalue),
  7              input[idx]
  8          )
  9      end
>10      return combined
 11  end

About to run: return RGB{Normed{UInt8,8}}[RGB{N0f8}(1.0,1.0,1.0) RGB{N0f8}(1.0,1.0,1.0); RGB{N0f8}(1.0,1.0,1.0) RGB{N0f8}(1.0,1.0,1.0)]
1|julia> combined
2×2 Array{RGB{N0f8},2} with eltype RGB{Normed{UInt8,8}}:
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)

[timholy]

Weird Julia bug. Works incorrectly on 1.0 and 1.2, but correctly on 1.3 and 1.4/nightly. I'll file a Julia issue and see if anyone knows which commit(s) fixed this and whether it's backportable.

[kimikage]

BTW, given the purpose of ifelse, the usage of ifelse seems to be bad manners.
Why don't you use combined[idx] = useother[idx] ? RGB{Float64}(othervalue) : input[idx]?

[galenlynch]

BTW, given the purpose of ifelse, the usage of ifelse seems to be bad manners.

I don't understand how this is bad manners?

The reason for using ifelse instead of ? is described succinctly in its documentation: https://docs.julialang.org/en/v1/base/base/#Core.ifelse

Basically, it eliminates a branch in the code. My hope was to make the compiler vectorize the inner loop, which it wouldn't do if I used ? or if instead of ifelse.

[galenlynch]

Unrelated, but @timholy congrats on the beautiful paper in Science! I'm reading it right now.

[kimikage]

I don't understand how this is bad manners?

The reason is exactly what you wrote.
First, to eliminate branches, you should not pass anything which could generate a branch to the ifelse arguments. Secondly, the argument types should be unified. Implicit type conversion means that it may generate branches.
Of course, this is a bug of Julia and your code is syntactically acceptable. In addition, whether branches are actually generated is another matter. However, your code can mislead the reader (at least me).

[galenlynch]

A bug is a bug is a bug. You're right that this particular example is a strange use of ifelse, which only serves to demonstrate the bug that I came across and couldn't explain. It's probably an error with Julia itself. If you're asking why I would want to have type unstable ifelse, the answer is that I didn't want that. However, I explicitly mentioned the type instability in the original post.

What actually happened is that I wrote some quick code without sufficient type bounds, and after using it for a while gave it an argument that resulted in the ifelse becoming type unstable. Instead of just adding branches to the code and making the code take longer to run, it produced a very surprising and very wrong result. The code above was my minimal reproducible example of that bug, after playing around with the issue and figuring out exactly when this bug would be triggered.

[kimikage]

Let me add only one point. If you think I pointed only to the type instability, it is a misunderstanding.
The type instability is another matter. In fact, Julia's compilers work correctly up to the type inference (i.e. Union{RGB{Float64}, RGB{Normed{UInt8,8}}}). I think this is an important nuance for people who learned of this issue from JuliaLang/julia#34231.
(Moreover, the stack allocation of 24 bytes for RGB{Float64} is also done correctly. However, for some reason, only the first 8 bytes are stored. The remaining 16 bytes are stored after everything is done.)

Even if it is type stable, we should not use implicit (unexpected) conversions within ifelse. The "bad manners" may have been exaggerated, but IMO, the unreasoning use of ifelse is not good practice even in quick code.

Edit:
You may say "Mind your own business". Unfortunately, Normed and color conversions are my own business.:smile:

[johnnychen94]

No offense, we could leave this issue as the tracker of this bug, and open a new issue for "best practice in FixedNumbers and documentation" if there's a need. :D

Edit:
I said this because I felt you two were arguing in two different things which I both agree with 😢

[galenlynch]

@johnnychen94 sorry to add more noise, but I actually don't understand the point that kimikage is trying to make.

@kimikage I don't understand your distinction between type instability and implicit type conversion within ifelse. Is your point that the two arguments after the condition to ifelse should always be the same type? Or is it instead that the resulting type of the ifelse function call should be the same as the location the result is being assigned to, which is what I would call implicit type conversion?

If you mean the second, then it seems like type conversion is not necessary to trigger what's probably the same bug in Julia 1.2 (inspired from your post in JuliaLang/julia#34231):

julia> for i = 1:2 # julia1.2
       display(ifelse(i > 1, 1//3, -1.0))
       end
-1.0
1//140412727051408

[kimikage]

What I wanted to say in #814 (comment) is that "my manners" should not be confused with the cause of this bug. My manners are something like "Type names should start with a capital letter."

Is your point that the two arguments after the condition to ifelse should always be the same type?

If "your point" means "my manner", the answer is "Yes". If it means the point of "Let me add only one point", the answer is clearly "No". Since ifelse is a function, such a special rule should not exist.

If you mean the second, then it seems like type conversion is not necessary to trigger what's probably the same bug in Julia 1.2

I don't know how to display numbers without conversion. Anyway, I don't fully understand the cause of the bug. (I'm not really interested in it now, because I confirmed that my modified code is not the cause.) Perhaps the conversions themselves are done correctly. Storing the intermediate variables (i.e. a part of the Φ function) seems to be failed.

[timholy]

This works as expected in Julia 1.9.