[kimikage]

This is a variant of PR #354 with deprecated fallback method retained.

While it would be hypocritical not to call this a breaking change, it does allow us to avoid a direct violation of SemVer.
In other words, I plan to release this in the v0.9 series.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.74%. Comparing base (d62abf3) to head (bd93684).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
- Coverage   96.75%   96.74%   -0.01%     
==========================================
  Files           7        7              
  Lines         800      799       -1     
==========================================
- Hits          774      773       -1     
  Misses         26       26              

☔ 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.

[PatrickHaecker]

I think this would currently violate the promised interface, but the fix seems plausible. LGTM with the fix.

I still haven't understood why this is better than just doing a 0.10 release in the next days, but that is irrelevant in terms of correctness.

[PatrickHaecker]

Please add few non-trivial Union cases, too.

[PatrickHaecker]

The docstring had some weaknesses and should describe the new situation explicitly:

Suggested change

	"""
	floattype(::Type{T})
	floattype(x::FixedPoint)
	Return a minimal type suitable for performing computations with instances of type `T` without integer overflow.
	For a `Real` type `T`, the returned type is a subtype of `AbstractFloat`. In general it has the minimum bitwidth
	needed to encode the full precision of `T`. However, computational efficiency takes priority, so the returned type
	may be wider. For example, `floattype(N0f8)` is currently `Float32` rather than `Float16`.
	For a non-concrete `FixedPoint` type, the returned type suits all of its concrete subtypes. A `Union` of `FixedPoint`
	types gives the promoted type of its members. Any other non-concrete `FixedPoint` type gives `floattype(R)` if its raw
	type `R` is concrete, and `BigFloat` otherwise.
	Methods are defined for `AbstractFloat`, `Integer`, `Rational`, `AbstractIrrational` and `FixedPoint` types. Other
	`Real` types currently return `T` itself with a deprecation warning, and will throw a `MethodError` in a future
	release. Packages may extend `floattype` to types that are not `Real`, as long as the returned type supports
	computation without integer overflow.
	# Examples
	A classic usage is to avoid overflow by converting `FixedPoint` values to a floating-point type:
	```jldoctest; setup = :(using FixedPointNumbers)
	julia> x = N0f8(1.0)
	1.0N0f8
	julia> x + x # overflow
	0.996N0f8
	julia> T = floattype(x)
	Float32
	julia> T(x) + T(x)
	2.0f0

Non-concrete FixedPoint types:

julia> floattype(Normed{UInt8})
Float32

julia> floattype(Union{N0f8, N0f16})
Float32

julia> floattype(Normed)
BigFloat

The following represents a valid extension of floattype to types that are not subtypes of AbstractFloat:

julia> using FixedPointNumbers, ColorTypes

julia> floattype(RGB{N0f8})
RGB{Float32}

RGB itself is not a subtype of AbstractFloat, but unlike RGB{N0f8}, operations with RGB{Float32} are not subject
to integer overflow.
"""

[kimikage]

Aside from the fact that it's long, I think the proposal isn't bad.
However, I don't understand exactly what purpose it serves.
This document is primarily intended for package developers. I don't know of any examples where floattype is actually being used outside of ColorTypes and its downstream packages.

I think the top-down definition of floattype should be discussed in JuliaLang/julia rather than in this package.

[PatrickHaecker]

The purpose for me was having documentation for the code which is aligned with each other. I agree that it is verbose, but I am not sure where it can be made shorter without losing something.

[kimikage]

I feel like detailed documentation ends up losing something.
If "The behavior of Union and UnionAll is subject to change until v1.0," who ends up losing out?

[kimikage]

@PatrickHaecker
Before we get into the implementation discussion, please let me know where you're willing to compromise.
I'm fine as long as an AbstractFloat tyoe is returned and it doesn't conflict with the current implementation for concrete types.

[PatrickHaecker]

@kimikage
The problem is that as long as the call fails, we can later define whatever we want without a breaking release. As long as it is deprecated, we can at least do it in a nicer way with a (single) major release. As soon as we commit to BigFloat, that ship has sailed (and deprecating and changing it should be two major releases). So I'd like each result we introduce now to be the one we want to keep.

Union{N0f8, Float32} is not a FixedPoint, so it stay in my proposal on the deprecated Real fallback.

The promote decision is one of the most important one, as this is so often called implicitly. BigFloat really should be the last resort, as it's immediately allocating and therefore at least of the kind of real-time programming I am mostly involved in a direct show-stopper. As FixedPointNumber is especially useful when performance matters, that is a hard sell.

Base only seems to promote to BigFloat if either BigInt or BigFloat are already involved. So even

julia> promote_type(Int128, Float16)
Float16

That seems a bit extreme to me, but that is what we should go for if we want to be as compatible as possible to Base.

Base is indeed very peculiar:

julia> promote_type(Union{Int8, Int16}, Float32)
Real

So we probably don't want to strive for maximum Base compatibility here, to not reproduce the bugs Base has:

w = vcat(Int8[100], Int32[3])
w[1] + w[1]                  # 200: the Int8 was converted to Int32

w = vcat(Int16[100], Int32[3])
w[1] + w[1]                  # 200: the Int16 was converted to Int32

a = Union{Int8, Int16}[Int8(100)]
v = vcat(a, Int32[3])        # eltype Signed, elements Int8, Int16, Int32
v[1] + v[1]                  # -56: Int8 arithmetic overflows

I reported that. Should we maybe see whether that report provides any insights?

[kimikage]

First of all, since this package is still at v0.x, I don’t think it’s necessary to mark something as deprecated before introducing breaking changes.
In fact, the promotion rules were changed between v0.8 and v0.9.
(I was absent from the decision-making process for the v0.9.0 release.)

I thought you were eager to switch from v0.9.x to v0.10, so I'm not quite sure what your priorities are anymore.
Also, I don't understand what you're trying to resolve in JuliaLang/julia#63922, or exactly how it affects the rules in this package.

My intention is, at the very least, to ensure that floattype() returns an AbstractFloat type for typical FixedPoints, and to keep the promotion rules for FixedPoints self-contained within this package (cf. PR #341).

IMO, the situation that floattype, which serves as the basis for FixedPoint's promotion rules, (indirectly) interferes with the rules seems like a warning sign for the entrance to a rabbit hole.

In light of PR #341, what should be my next step?

[PatrickHaecker]

since this package is still at v0.x, I don’t think it’s necessary to mark something as deprecated before introducing breaking changes.

That's a good point. I was, however, more focused on when we want to change it in the future. And that might very well be 1.x land.

My intention is, at the very least, to ensure that floattype() returns an AbstractFloat type for typical FixedPoints, and to keep the promotion rules for FixedPoints self-contained within this package (cf. PR #341).

But why then promoting to BigFloat? Or do you mean self-contained within the set of this package and Base?

IMO, the situation that floattype, which serves as the basis for FixedPoint's promotion rules, (indirectly) interferes with the rules seems like a warning sign for the entrance to a rabbit hole.

Would it make sense to decouple floattype from FixedPoint?

In light of PR #341, what should be my next step?

Would it help to explain what's the purpose of #362? I think it would if I could understand what problem should be solved and why BigFloat is the best way to achieve this.

[kimikage]

Would it help to explain what's the purpose of #362?

As mentioned above, the purpose is to ensure that floattype() for FixedPoint returns AbstractFloat.
Without that guarantee, flaws in the promotion rules will propagate.
I am not claiming that BigFloat is the best option, nor do I believe it is.
It’s simply that BigFloat is the biggest AbstractFloat we can handle.

[PatrickHaecker]

the purpose is to ensure that floattype() for FixedPoint returns AbstractFloat.
Without that guarantee, flaws in the promotion rules will propagate.
I am not claiming that BigFloat is the best option, nor do I believe it is.

Ah, okay, wouldn't then the best option be to use the code as is and explicitly mention in the docstring that whenever BigFloat is returned, this can change in the future at any time? So that it does not get part of the contract? I think this would combine the best of both worlds: Not implying that this is the solution we want to have in the future and unblocking progress.