Improve inferrability

[timholy]

This package is part of ImageCore, and because ImageCore is essentially the foundation of JuliaImages my overall goal is to set a high bar on the "engineering quality" of all packages up through ImageCore. Good inferrability = good precompilability = low latency (and good runtime performance, of course) for anyone who is building stuff based on ImageCore.

This package was the source of the large majority of inference failures while running ImageCore's test suite. This doesn't eliminate all failures but it's a pretty huge step forward: in this package's tests, it cuts it from >350 inference failures to around 100. Splatting a vector-of-arrays accounts for a good chunk of the rest, and of course no one actually has to do that (just leave as a vector-of-arrays, or construct as a tuple in the first place), so overall this seems pretty solid now. The eltype inferrability in particular is much better.

This PR is the motivation for JuliaArrays/OffsetArrays.jl#189

[timholy]

These are designed to be specialized by ImageCore. Because CI runs ImageCore's tests, this will have test failures until ImageCore gets updated.

[johnnychen94]

I'm not sure the name is great, perhaps that should be changed.

Perhaps heterogeneous_promote_type or wrapped_promote_type (Colorant as a wrapper of number, and Union{Missing, T} as a wrapper of T)?

[timholy]

This drops the time to run the tests from ~18s to ~14s on my machine.

[timholy]

For the test failures, note that it failed on the ImageCore test, which isn't surprising since ImageCore isn't compatible with version 0.3 of this package. It installed an old version of ImageCore (0.8.12) from before it depended on this package.

[johnnychen94]

I'm happy to separate MosaicViews out from ImageCore@1.0, if you find it necessary. It's just that ImageCore is the most appropriate place to reexport this package so far; the less appropriate place to reexport this is in ImageShow.

[timholy]

I don't think it's necessary, as long as it's solid---we don't have to have zero inference failures (we can't), but I'd like to get rid of "egregious" cases. As long as we've done that, I'm fine having this in ImageCore if you think it makes sense.

That said, can you briefly remind me of why it's better in ImageCore than ImageShow? I'm not sure I was paying attention when that decision was made. With regards to what should go in ImageCore, there's also this request: JuliaImages/ImageTransformations.jl#69

What do you think of these changes? Does it make it good enough? The part to check most carefully is this section:

MosaicViews.jl/src/MosaicViews.jl

Lines 418 to 436 in 861b20c

	_gettype(A::AbstractArray{T}) where T = T === Any ? typeof(first(A)) : T
	# This uses Union{} as a sentinel eltype (all other types "beat" it),
	# and Bool as a near-neutral fill type.
	_filltype(As) = PaddedViews.filltype(Bool, _filltypeT(Union{}, As...))
	@inline _filltypeT(::Type{T}, A, tail...) where T = _filltypeT(promote_type_withcolor(T, _gettype(A)), tail...)
	_filltypeT(::Type{T}) where T = T
	# When the inputs are homogenous we can circumvent varargs despecialization
	# This also handles the case of empty `As` but concrete `T`.
	function _filltype(As::AbstractVector{A}) where A<:AbstractArray{T} where T
	(!@isdefined(T) || T === Any) && return invoke(_filltype, Tuple{Any}, As)
	return PaddedViews.filltype(Bool, T)
	end
	promote_type_withcolor(::Type{T}, ::Type{S}) where {T, S} = promote_type(T, S)
	promote_type_withcolor(::Type{T}, ::Type{Union{}}) where T = T
	promote_type_withcolor(::Type{Union{}}, ::Type{S}) where S = S
	promote_type_withcolor(::Type{Union{}}, ::Type{Union{}}) = Union{}

The key change, aside from inferrability, is "opt-in" with respect to fixing any issue that promote_type doesn't handle on its own. That means that any package definite new types that don't automatically promote with others needs to specialize the method. I'm not sure the name is great, perhaps that should be changed.

[johnnychen94]

Everything looks good to me. My original version is not ideal(it simply works for common cases JuliaImages/ImageCore.jl#135) and I like your "sentinel" version more; it is easier to understand and maintain/extend. Thanks for the fix.

That said, can you briefly remind me of why it's better in ImageCore than ImageShow? I'm not sure I was paying attention when that decision was made.

I decided to move mosaicview into ImageCore because:

1. PaddedViews is used by ImageCore to support color promotion(JuliaImages/ImageCore.jl#126), so adding MosaicViews doesn't add too much latency there.
2. MosaicViews is an enhanced version of cats and is something "basic". (Or, enhanced version of Matlab's montage and imshowpair)
3. MosaicView is a view, and ImageCore is a collection of traits and views.

[johnnychen94]

I'm not sure the name is great, perhaps that should be changed.

Perhaps heterogeneous_promote_type or wrapped_promote_type (Colorant as a wrapper of number, and Union{Missing, T} as a wrapper of T)?

[timholy]

All that sounds good. Initially I was wondering if the view part was mostly fake (mosaicview is eager), but then I see that the constructor MosaicView really is a view.

OK, I'll finish this off and merge. Then on to ImageCore!

[timholy]

One last thought: I think mosaicview (lowercase m) is a misnomer, because there's no sense in which it's a view: if you mutate any of the values in the result, it doesn't propagate back to the source arrays.

Should we call it mosaic?

EDIT: I'm talking specifically about the signatures mosaicview(As::AbstractVector{<:AbstractArray}; kwargs...) and mosaicview(As::Tuple; kwargs...), not mosaicview(A::AbstractArray{T,N}; kwargs...). The latter could continue to exist since IIUC it's a real view. This applies just to the code paths that call _padded_cat.

EDIT2: the most recent commit adds that deprecation

[johnnychen94]

After modifying the examples in README.md, it would be good enough to merge.