Add widen for FixedPoint types

[PatrickHaecker]

Widening the raw type and keeping the number of fractional bits makes + and - safe from overflow, as Base.widen requires, and makes the generic widemul work for fixed-point numbers. 128-bit raw types stay unsupported because widen(Int128) is BigInt, which is no valid raw type.

The widening logic is the one which is closest to what Base does. However, if users are not happy, they can still add more specific methods which override this behavior for their use cases.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.85%. Comparing base (b863f66) to head (d6729d2).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
+ Coverage   96.83%   96.85%   +0.01%     
==========================================
  Files           7        7              
  Lines         791      795       +4     
==========================================
+ Hits          766      770       +4     
  Misses         25       25              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kimikage]

Personally, I think it is a reasonable proposal.
However, I am not certain whether there is a consensus on fixing f.
Since this package serves as a foundation for the JuliaImages ecosystem, the argument that N0f16 is the widened type of N0f8 also seems reasonable to me.

In short, I think a docstring is necessary.

And what is even more problematic is the fallback implementation of widemul.

julia> widemul(1N0f8, 1N1f7)
1.0N24f8

There is clearly no consensus on f.
I believe widemul should explicitly throw an error for now (at least, in the case where f1 != f2 Edit: There also seems to be room for debate regarding the f1 == f2 case.).

[kimikage]

This should not be merged until the discussion regarding widemul is resolved.

[PatrickHaecker]

However, I am not certain whether there is a consensus on fixing f. Since this package serves as a foundation for the JuliaImages ecosystem, the argument that N0f16 is the widened type of N0f8 also seems reasonable to me.

There is hardly any degree of freedom, because Base's docstring defines it:

If x is a type, return a "larger" type, defined so that arithmetic operations + and - are guaranteed not to overflow nor lose precision for any combination of values that type x can hold.

For fixed-size integer types less than 128 bits, widen will return a type with twice the number of bits.

So widen(N0f8) == N0f16 would be incorrect.

I agree that there are use cases where other definitions are more useful. However, they either need to use a different function (widenfrac?), a different method (widen(Int32, 1) would make sense, as would widen(N0f8, 0, 8) == N0f16) or they need to define a more specific method.

In short, I think a docstring is necessary.

Base mostly has it covered, but I added a short one to make it really explicit.

And what is even more problematic is the fallback implementation of widemul.

julia> widemul(1N0f8, 1N1f7)
1.0N24f8

There is clearly no consensus on f. I believe widemul should explicitly throw an error for now (at least, in the case where f1 != f2).

I think that's clear. Let X1 and X2 have d1 and d2 decimal bits and f1 and f2 fractional bits. For y::Y = x1::X1 * x2::X2, Y should have d1 + d2 decimal bits and f1 + f2 fractional bits.

However, that's not the focus of this PR, so I made it throw following your suggestion.

[kimikage]

The definition of widemul that you consider "clear" is not obvious to me. (This does not mean you are wrong; it simply means there is ambiguity there.)
So, I believe treating it as an error for the time being is the right thing.

[PatrickHaecker]

So, I believe treating it as an error for the time being is the right thing.

Yes, definitely for this PR. However, the discussion about that might be useful to bring a potential future PR forward.

The definition of widemul that you consider "clear" is not obvious to me. (This does not mean you are wrong; it simply means there is ambiguity there.)

Where do you see the ambiguity? This is how multiplication of fractional values works in binary numbers:

• multiplying two numbers with d1 and d2 (possibly leading zeros, possibly sign-including) integer digits, the result will have at most d1 + d2 integer bits.
• multiplying two numbers with f1 and f2 (possibly trailing zeros) fractional digits, the result will have at most f1 + f2 fractional bits.

So the only general, type-safe definition (independent of the values) which is aligned to "doubling the number of bits from Base" is the definition I provided.

[kimikage]

However, the discussion about that might be useful to bring a potential future PR forward.

I think it would be better to submit a separate issue. This PR will be merged before a conclusion is reached in the discussion on widemul.

[PatrickHaecker]

Thanks for all the work you put into the package and all the reviews. Ready for the next review iteration.

[PatrickHaecker]

I think it would be better to submit a separate issue.

Done: #353