add a check for valtypename to comparedims to be used when broadcasting#879
add a check for valtypename to comparedims to be used when broadcasting#879tiemvanderdeure wants to merge 2 commits intorafaqz:mainfrom
comparedims to be used when broadcasting#879Conversation
comparedims to be used when broadcasting
|
Isn't this too strong? Now Which may not be what we want? |
|
Okay probably not. So maybe we want to check if they have the same abstract type. So if both are |
|
Yeah, this is probably why I didn't do it 😅 Like what does the same abstract type really mean when it's generally accepted that you can add intermediate abstract types without a breaking change. |
| isnothing(msg) || _orderaction(msg, a, b) | ||
| return false | ||
| end | ||
| if valtypename && Base.typename(typeof(parent(a))) != Base.typename(typeof(parent(b))) |
There was a problem hiding this comment.
We could introduce a function for this check and return false as fallback and return true for combinations that assume to be comparable
There was a problem hiding this comment.
Still a bit complicated to know what the function should do.
We may need to just manually distinguish AbstractSampled from AbstractCategorical, and let promote_dims take care of the rest
There was a problem hiding this comment.
Maybe this function could return true for identical types, or for combinations where there is some specialized implementation of promote_first (which is I think just for both AbstractSampled or two AbstractCategorical)
There was a problem hiding this comment.
The reason it returns this NoLookup currently is because nothing else is implemented for promote_dims/promote_first (I think?), so it would make sense to kind of mirror that.
There was a problem hiding this comment.
Yeah. Its a bit odd to have the same thing in two places... I wonder if we can somehow make it share the same code.
There was a problem hiding this comment.
Like have this comparison dependent on the results of promote_first
Solves #878
E.g.
With the error message
ERROR: DimensionMismatch: Lookup types for Z do not match: Sampled and Categorical.