-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP/RFC/Scary: allow signed Normed numbers #143
base: master
Are you sure you want to change the base?
Conversation
Wow, this is a great challenge! From a practical point of view, since Edit: |
Not entirely. I don't entirely subscribe to what follows, but for the purposes of argument let me make the extreme case. Consider
If magnitude matters, it's bad when With this line of thinking, there are two options: (1) deliberately lose one bit of precision, and for an 8-bit image demote it to 7-bit and then use the extra bit as a sign bit (so that 8-bit images get loaded as Again, I'm making the "extreme" case to show why this is worth considering. A more balanced view would point out two important negatives:
The latter is a problem, but the fact that it's limited to a doubling (and not, say, 8x) and that RAM is cheap means that I don't think the latter is fatal. I think the former is the more serious disadvantage of such a change. If you ask me, what is my honest opinion, I am not sure. I support the fact that operations with |
We should make sure that major contributors to/watchers of JuliaImages are aware of this discussion: CC @kmsquire, @rsrock, @SimonDanisch, @Evizero, @tejus-gupta, @annimesh2809, @zygmuntszpak, @RalphAS, @Tokazama, @juliohm, @bjarthur, @ianshmean, @jw3126, @tlnagy. Sorry for the spam (and sorry if I omitted someone who might want to know). |
I'm skeptical of its practical usage.
unless all operations are done under # not returning N0f8 for N0f8 input
img_in = load(imgfile) # N0f8
img_temp = foo1(img_in) # N0f8->Float32
img_out = foo2(img_temp) # Float32
img_out = n0f8(img_out) # Float32->N0f8 Note that there're only two conversions. But if it became: # returning N0f8 for N0f8 input
img_in = load(imgfile) # N0f8
img_temp = foo1(img_in) # N0f8->Float32->N0f8
img_out = foo2(img_temp) # N0f8->Float32->N0f8 There're four now, where two of them are unnecessary. If I understand it right, what you're trying to do here is purely replacing I once opened an issue related to what I said here (preserving type): JuliaImages/ImageCore.jl#82. |
It's been a while since I deeply thought about FixedPointNumbers and the difference between Fixed and Normed.
I see there are two orthogonal concerns.
From a representational perspective I don't think there is any strong reason to not have signed normed numbers, although looking at my comment history I once remarked that I had no longer a strong reason for having unsigned fixed point numbers. The normed numbers always had an odd place in the ecosystem, since I see them primarily as storage numbers and not as computational numbers. In conclusion I don't have strong opinions and only existential questions. It might make sense to divorce fixed and normed and make normed an explicit color representational/computational type instead of having to worry about general semantics. |
From @johnnychen94 :
and @vchuravy:
Yep. Both of these express the notion that the current values are mostly storage and not computation. If you already think this way, there's little advantage to this proposal. However, we do support computation with such values. Since I think I may not have been sufficiently clear, let me walk you through an interesting experiment. This is one that someone might do to compare two images, or look for frame-by-frame temporal differences in a movie: julia> using ImageView, TestImages
julia> mri = testimage("mri");
julia> diff1 = mri[:,:,1:end-1] - mri[:,:,2:end];
julia> diff2 = float.(mri[:,:,1:end-1]) - float.(mri[:,:,2:end]);
julia> imshow(diff1); imshow(diff2) It's worth doing this on your own, but to make life easy here are the first frames from each: The second one is "correct," with mid-gray encoding no difference. Or you can use (magenta is positive and green is negative) Now, if you're used to calling To the specifics of this proposal: aside from defining & supporting the type (which, as @kimikage indicated, is a burden), the change that would "fix" these issues is a single line, which could be written: -(x::N0f8, y::N0f8) = S7f8(Int16(x.i) - Int16(y.i), 0) This is definitely a breaking change, but it's not a huge computational burden: julia> minus1(x, y) = x - y # this is essentially what we do now
minus1 (generic function with 1 method)
julia> minus2(x, y) = Int16(x) - Int16(y) # this is what we could do if we had S7f8
minus2 (generic function with 1 method) when passed julia> @btime minus1(x, y) setup=(x = rand(UInt8); y = rand(UInt8))
1.240 ns (0 allocations: 0 bytes)
0x22
julia> @btime minus2(x, y) setup=(x = rand(UInt8); y = rand(UInt8))
1.239 ns (0 allocations: 0 bytes)
187 So in contrast to what @johnnychen94 said above, there isn't much/any of a conversion overhead to worry about. (For comparison, Now, just about the only operation this would affect are + and -; as soon as you multiply by a So, to recap, the costs of this are:
and the benefits are
That's pretty much the bottom line, I think. |
I'm satisfied with the "breaking change" and it's more like bug fixes to me. An alternative way to fix this issue is making |
As I showed in the past benchmarks, the benchmarks of a single operation can easily mislead us.:confused: |
function _convert(::Type{U}, x::Tf) where {T, f, U <: Normed{T,f}, Tf <: Union{Float32, Float64}} | ||
if T == UInt128 && f == 53 | ||
0 <= x <= Tf(3.777893186295717e22) || throw_converterror(U, x) | ||
function _convert(::Type{N}, x::Tf) where {T <: Integer, f, N <: Normed{T,f}, Tf <: Union{Float32, Float64}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry but this method is currently optimized for non-negative numbers and cannot handle negative numbers. (pay attention to k
.) Of course, this is a trivial issue, but I mean, "optimization is specialization".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I hadn't checked any of this, this is WIP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't care about that much in FixedPointNumbers
. However, it is generally not good to ask other packages for such verification and modification. If this is essential to the design, we should change it aggressively, but I don't think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand. All I meant was that this was a draft I decided to let people know I was working on. I will fix this before we merge this if we decide to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a simple problem. The current Normed
is always unsigned, but your Normed
is not always unsigned.
@@ -241,7 +266,8 @@ Base.Rational{Ti}(x::Normed) where {Ti <: Integer} = convert(Ti, reinterpret(x)) | |||
Base.Rational(x::Normed) = reinterpret(x)//rawone(x) | |||
|
|||
# Traits | |||
abs(x::Normed) = x | |||
abs(x::Normed{<:Unsigned}) = x | |||
abs(x::T) where T<:Normed = T(abs(x.i), 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does not abs(::Normed{<:Signed})
return Normed{<:Unsigned}
? (Of course, this is not a question.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try this: abs(Int8(-5))
. In general Julia doesn't change the number type, even to the point where abs(typemin(Int8))
returns a negative number!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. Therefore, it can be concluded that -(x::N0f8, y::N0f8) = S7f8 (Int16(x.i) - Int16(y.i), 0)
is not good! 😝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this is in conflict with the rest of Julia. I guess we could return Normed{<:Unsigned}
, since the only other "always agrees with expectations" is to widen which also changes the type.
It might be better to clarify what we need.
It is premature to discuss the speed because the current Edit:
I also agree about that. So, I think there are two types of the type hierarchy: Fraction-first
Sign-first
I think it is a good idea to add the signed |
Yeah I have a strong preference for the first one since it emphasises the semantic difference between Normed and Fixed. |
Since we can only choose one for now, I agree completely. However, I think choosing the former is not denying the latter. If we explicitly choose the former, we should be responsible for the implicit nature of the latter. |
Not sure I fully agree here. We allow folks to construct out-of-gamut colors routinely. This is important in, e.g., computing the mean color of an image patch, where the natural implementation of mean is
I wasn't sure which place, can you elaborate? I don't think anything in the concept of "normed" enforces "unsignedness," so I don't see any problem with |
I'm sorry. I'm not good with words.
What I want to say is not about the gamut, but about the visualization. Since our LCDs cannot represent negative RGB colors, we need to project abstract numbers into concrete colors. It usually involves the type conversion. If you do type conversion after all, is there any reason not to convert the numbers to a more convenient type (e.g. Although signed |
Good! This focuses the discussion down. Really I think we have 3 options:
|
In comparing 2 vs 3, perhaps the key question is whether the extra complexity is worth optimizing a small number of operations (basically, addition and subtraction). For reference, julia> function mysum(acc, A)
@inbounds @simd for a in A
acc += a
end
return acc
end
mysum (generic function with 1 method)
julia> using FixedPointNumbers
julia> A = collect(rand(N0f8, 100));
julia> using BenchmarkTools
julia> Base.:(+)(x::N56f8, y::N0f8) = N56f8(x.i + y.i, 0)
julia> @btime mysum(zero(N56f8), A)
23.733 ns (1 allocation: 16 bytes)
50.255N56f8
julia> @btime mysum(0.0f0, A)
23.544 ns (1 allocation: 16 bytes)
50.254906f0
julia> @btime mysum(zero(N56f8), A)
22.852 ns (1 allocation: 16 bytes)
50.255N56f8
julia> @btime mysum(0.0f0, A)
23.782 ns (1 allocation: 16 bytes)
50.254906f0 so at least on my machine the performance difference seems to be within the noise. (I am surprised, TBH.) Can others try this benchmark and report back? |
These trends apply not only to x86_64 but also to ARM (with NEON). julia> versioninfo()
Julia Version 1.0.3
Platform Info:
OS: Linux (arm-linux-gnueabihf)
CPU: ARMv7 Processor rev 4 (v7l)
WORD_SIZE: 32
LIBM: libopenlibm
LLVM: libLLVM-6.0.0 (ORCJIT, cortex-a53)
julia> @btime mysum(zero(N56f8), A)
1.219 μs (2 allocations: 32 bytes)
47.776N56f8
julia> @btime mysum(0.0f0, A)
1.698 μs (1 allocation: 8 bytes)
47.776474f0 Of course, the 32-bit accumulator is faster. julia> Base.:(+)(x::N24f8, y::N0f8) = N24f8(x.i + y.i, 0)
julia> @btime mysum(zero(N24f8), A)
757.864 ns (2 allocations: 16 bytes)
47.776N24f8 |
OK, so there are architectures where it makes a 2x difference. Is that (along with its domain of applicability, which is basically summation/differencing) enough to matter? |
I think this is a compiler issue rather than a CPU architecture or CPU performance issue. Of course, compiler-friendly types are better. However, is that really what you wanted to know with the benchmark? Edit: Edit2: |
There are two orthogonal concepts for accumulation.
julia> function mysum_raw(acc, A)
@inbounds @simd for a in A
acc += reinterpret(a)
end
return acc / FixedPointNumbers.rawone(eltype(A))
end This is just a conceptual code and not practical. This difference might be noticeable for large (more practical) arrays.
|
I don't think most algorithms should dive into this level of detail about their element types. Your code above should be equivalent to |
My conceptual code is never never never practically good. Is it related to this discussion? 😕 |
#143 (comment) is, in my mind (flaws included), a benchmark for whether 3 provides any advantage over 2. Given that you don't like option 2, and that at least on some architectures 3 does perform better than 2, it seems our main options are 1 and 3. Option 3 would allow one to fix #41, #126, JuliaImages/ImageCore.jl#63. Option 1 is the status quo. Can we agree on 3 as a reasonable consensus solution? |
First of all, a wrong benchmark misleads us. Since I think the main problem of #41 (and JuliaImages/ImageCore.jl#63 ?) will be solved by What are the specific problems which Option 1 cannot solve? |
I don't doubt you already know everything I'm about to say, so I may not be understanding your true question. But let me try: Let me try to get others to chime in by making the options here crystal clear:
My hope in submitting this PR was that option 3b would "thread the needle" and be an acceptable option to everyone. I think that's pretty clearly not the case. If I understand the voting so far, it looks like this (:+1: means approve, :-1: means disapprove, nothing means relatively neutral):
If I've misunderstood people's votes, please correct me. How do others vote? |
Your explanation is very easy to understand. 👍 Option 2: I don't know much about the costs of education, but generally it's not bad to be able to manage your own workspace yourself. Whether the answer is correct or not is determined by the specifications, not by the absence of "overflow". In the first place, where do |
To clarify, option 2 = "force conversion" is equivalent to "delete the code supporting arithmetic." The only way to force users to convert to some other type before doing arithmetic is for arithmetic on
I've checked every package in JuliaRegistries/General that depends on FixedPointNumbers. The only one that isn't related to image processing or colors is https://github.com/JuliaAudio/SampledSignals.jl, which from their README would apparently benefit from |
Option 2: neutral 😑
If it's on test stage, Plug-and-play is a very common style when we're processing images. As a playground, we generally don't write tests. When wrap around behavior happens without detected, we would tend to conclude that "my processing pipeline should be modified" and it's actually quite severe mislead in many research scenarios.
I only manually convert Float32 to Normed when I need to benchmark different algorithms -- take advantages of the inaccuracy of representation -- just to have a more stable benchmark result, e.g., julia> float_x = 0.1
0.1
julia> float_y = 0.103
0.103
julia> N0f8(float_x) - N0f8(float_y)
0.0N0f8 |
@johnnychen94, thanks for clarifying; I've updated your votes in the table accordingly. One thing I should also make sure is understood: Python's Consequently I understand @johnnychen94's dissatisfaction with the current status quo, even if I also acknowledge that it's one of the most self-consistent options we have. |
I understand and clearly disagree with Option 2.
I cannot completely agree with that. I just proposed another way. (The way is not pretty good, though.)
Great survey! However, isolated signed
That's right! Therefore, I think testing is very important. A program with no specifications or tests is a poem. Although I do never hate poetry, it is not the basis for software design. This is just a thought experiment. What about Edit: |
What is NormedFloat?
It can go on paralyzing development forever, probably 😉. Seriously, we need to reach some kind of consensus here. Why don't you propose what you think we should do, rather than just argue against what I am proposing? AFAICT the only solution currently is the status quo. I can live with that but I'm not happy with it. Perhaps you feel you have proposed a solution, but if so I didn't understand it. EDIT: To be concrete, I was hoping to put in a grant on Dec. 17th to try to fund work on JuliaImages. It would be great to reach consensus well before that. |
I see you edited your post to clarify. Yes, it's fine to throw an informative message. I would have done it that way, I was being a bit casual by saying we'd just delete them. Delete the content, I should have specified. IUUC, given a friendly error message you are favorable on Option 2, no? I am confused by the reply above, however. I'll amend your votes once I understand your views correctly. |
I'm sorry for the lack of explanation, but it is so simple that no explanation is needed. |
I don't think it is nice, but narrowing the entry pathway is one solution to "force conversion" , isn't it? julia> img = load("mypicture.png")
┌ Warning: Please specify the pixel type, e.g, `load(Float32, "mypicture.png")`. You are using `N0f8` now.
└ @ Foo
2×2 Array{RGB{N0f8},2} with eltype RGB{Normed{UInt8,8}}:
RGB{N0f8}(0.867,0.157,0.169) RGB{N0f8}(0.984,0.133,0.851)
RGB{N0f8}(0.855,0.984,0.945) RGB{N0f8}(0.553,0.549,0.133) Again, I don't think this is nice. However, I'm not against this because this is the approach which is closer to the users than the low-level |
My biggest concern is that there are very few people participating in this discussion rather than what the final decision is. If you are convinced that signed I do not intend to inhibit it maliciously. However, I think "we should do" #139 and supporting |
Just FYI, I have been following this, but I’ve not felt like my thoughts are yet robust enough to contribute. Perhaps I’d just say a few things:
I guess it seems like some more fleshed-out working case studies could help frame the discussion? HTH! |
I can't see any advantage in NormedFloat: why have an implicit
We can't force conversion of a 1TB image. My lab uses those all the time, via memory-mapping. However, I suppose we could change all the image-loading routines to return a MappedArray using
Yes, we could create the type and then decide how to use it later. I just don't feel like creating types for the fun of it, I'd rather be sure there's a path towards them being useful, hence the proposal to change the rules of arithmetic. Breaking changes aren't a disaster if they are managed carefully; we can bound all packages that use FixedPointNumbers to 0.x versions, and then the packages that want to keep up can install CompatHelper to learn about the new approach. Breaking changes are acceptable especially for a 1.0 release; we broke just about everything in the Julia 0.6->1.0 transition. We already have some breaking changes queued, though any change in arithmetic rules would be a much larger change. So for me the point of this discussion was to determine whether we can find a better way. I am thinking we can't, so perhaps I should abandon this direction. |
@kimikage, I may have been remiss in not directing you to this earlier, but if you haven't read it yet I highly recommend reading https://juliaimages.org/latest/arrays_colors/, particularly starting at https://juliaimages.org/latest/arrays_colors/#fixedpoint-1 and reading to the end of the page. It's worth comparing to that scikit-image page I linked above, particularly section titled "Input types" which, you'll note, forces one to change the value when you want to change the representation. I apologize for the lack of a Japanese translation, I am aware that it makes it much more difficult. I would not fare well if the only documentation were available in Japanese 🤷♂️ . |
In the first place, please don't put the blame on my lack of knowledge. It is my personal problem that I am stubborn, so you can blame just me any amount. However, my (or our) claim is that we should keep it consistent with the rest of Julia. No matter how much I learn, the Julia language will never change unless I take action. |
This change would fix the problem, but we would lose lossless precision doing so and it would increase the cost of accessing a view of the image since you would need to do this conversion every time. Do we have benchmarks for how much lazy conversions like this would affect common operations? After reading through this thread, here are my thoughts:
* I think I've had to resort to Option 3a (recasting image
I would go further and say that we would be doing the greater imaging community a disfavor if we didn't try and develop something better given all the problems with the status quo and that we are in a unique position to build it.
I hope not! I like the bones here and the status quo could be better and changes like these are why I love the JuliaImages ecosystem :) |
FYI, I used a lot of
And in test stage, I use This approach is definitely not good in performance, but at that moment (got bit by wraparound behavior) I just wanted to guarantee accuracy first. This discussion is becoming too long to catch up with, so I hope consensus at some level can be reached soon, and I'd like to contribute to where I'm capable of. |
(Edit: Of course, Edit2:
So, you have a narrower gate, right? An important point in this approach is to prevent the (new) users from using
Ideally. Please face the facts(#139). We are only interested in what we need. I hate potential minor bugs. They are more vicious than obvious malfunctions.
Please give me some time to study. My claims have been already written, so it's okay to conclude this discussion during my absence. |
I do not mean to be rude, but the supporters of "3b" had better clarify the promotion rules. (cf. #147 (comment)) |
Been following this but don't have the bandwidth right now to help think upon solutions too much. The only thing I really care about on this is that:
I think both of these have been addressed to some degree, so the only other thing I'd add is that I only use fixed point numbers as a storage type passed to graphics libraries. Most of the time it doesn't make sense for an MRI to be directly represented as a fixed point number. |
This is a partial implementation of a speculative change, allowing signed variants of
Normed
. For example,S7f8
would represent the range-128.502
to128.498
; likeN0f8
it is still scaled by 255, but also has 1 sign bit and 7 bits to store absolute values bigger than 1.The motivation here is to support differences, related to #126, #41, and coming up with a good accumulator type in CheckedArithmetic.
Speculatively, we could use
S7f8
for subtraction (and maybe addition) withN0f8
. Pros: computing differences among pixels in images is a common operation, and it would be nice for this to be less of a gotcha than it is currently. Cons: this is against convention in the Julia ecosystem, and in opposition to #27. Thoughts? I'd especially like to hear from @vchuravy in addition to other recent contributors.