Fix pairwise for type-unstable corner case function #772
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`promote_type` is not a completely correct way of computing an upper bound for the return eltype. Use the same strategy as `map` and `broadcast` in Base instead, but with `promote_eltype` rather than `promote_typejoin`. We can still use `typejoin_union_tuple` since promotion does not happen inside tuple types. On Julia versions before 1.6 we would have to copy the full definition of `typejoin_union_tuple`, which is quite complex, so instead fall back to inferring eltype `Any` (inference wasn't so good anyway on these versions).
bkamins
reviewed
Mar 13, 2022
src/pairwise.jl
Outdated
| end | ||
| end | ||
| else | ||
| promote_type_union(::Type) = Any |
There was a problem hiding this comment.
do we need to be that defensive? If passed type is not a union then I guess we can return it - right?
There was a problem hiding this comment.
Good point. Actually the only case that would be hard to support is Tuple types. I've refactored the code to improve this.
bkamins
reviewed
Mar 13, 2022
bkamins
reviewed
Mar 15, 2022
| elseif T <: Tuple | ||
| return typejoin_union_tuple(T) | ||
| else | ||
| return T |
There was a problem hiding this comment.
Is line 148 covered in tests?
There was a problem hiding this comment.
It's covered as promote_type_union calls itself recursively, but I've added two tests for clarity.
bkamins
approved these changes
Mar 15, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
promote_typeis not a completely correct way of computing an upper bound for the return eltype. Use the same strategy asmapandbroadcastin Base instead (JuliaLang/julia#30485), but withpromote_eltyperather thanpromote_typejoin. We can still usetypejoin_union_tuplesince promotion does not happen inside tuple types.On Julia versions before 1.6 we would have to copy the full definition of
typejoin_union_tuple, which is quite complex, so instead fall back to inferring eltypeAny(inference wasn't so good anyway on these versions).Fixes #771.