-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add promote_typejoin() and use it instead of typejoin() to preserve Union{T, Nothing/Missing} #25553
Merged
Merged
Add promote_typejoin() and use it instead of typejoin() to preserve Union{T, Nothing/Missing} #25553
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
664cb99
Add special case for Void and Missing in typejoin() to return Union{T…
nalimilan 3ae9f50
Introduce promote_join() and use it instead of typejoin() where appro…
nalimilan d7c501c
Support promoting parameters of Tuple and NamedTuple, add promotion f…
nalimilan 39ceccb
Rebase fixes
nalimilan 8870ff5
Revert changes to tuples promotion
nalimilan 5c8deed
Remove incorrect @_pure_meta annotation
nalimilan 556e416
Rename promote_join to promote_typejoin
nalimilan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -237,7 +237,7 @@ eltype(::Type{TwicePrecision{T}}) where {T} = T | |
|
||
promote_rule(::Type{TwicePrecision{R}}, ::Type{TwicePrecision{S}}) where {R,S} = | ||
TwicePrecision{promote_type(R,S)} | ||
promote_rule(::Type{TwicePrecision{R}}, ::Type{S}) where {R,S} = | ||
promote_rule(::Type{TwicePrecision{R}}, ::Type{S}) where {R,S<:Number} = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this is right. For some reason |
||
TwicePrecision{promote_type(R,S)} | ||
|
||
(::Type{T})(x::TwicePrecision) where {T<:Number} = T(x.hi + x.lo)::T | ||
|
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
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
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
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
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
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
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
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.
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 docstring could be made clearer. I'm trying to figure out exactly what
typejoin
andpromote_typejoin
are doing and I don't think the current descriptions are complete. The current docstring fortypejoin
states that it returnsbut that must always be
Union{T,S}
. However, my understanding is thattypejoin
doesn't return unions and thatpromote_typejoin
was introduced exactly because of that. So what is this function returning? My guess would be something likeThere 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 I just copied "closest common ancestor" from a comment and tried to make it a little more explicit. You're right that
typejoin
returns an abstract supertype and never aUnion
AFAIK.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.
If the definition I stated above is correct then it might be fine to just define ancestor to cover only declared abstract types (and concrete types), i.e. exclude unions. It probably makes sense the declared subset of the type lattice is a tree so ancestor feels natural.
The name of the function would be slightly a misnomer as the lattice join is the least upper bound, but I guess some upper bound is fine for our use.
Examples would then also be helpful in the docstring. I wouldn't mind preparing the PR but I would need a confirmation that my understanding of the function is correct.