Skip to content
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.

Sign up for GitHub

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 a check for valtypename to comparedims to be used when broadcasting #879

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

add a check for valtypename to comparedims to be used when broadcasting #879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d5be5a
add a check for valtypename
tiemvanderdeure Dec 4, 2024
a1e042f
fix _valtypenamemsg function name
tiemvanderdeure Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/Dimensions/primitives.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,12 +558,18 @@ _valmsg(a, b) = "Lookup values for $(basetypeof(a)) of $(parent(a)) and $(parent
_dimsizemsg(a, b) = "Found both lengths $(length(a)) and $(length(b)) for $(basetypeof(a))."
_valtypemsg(a, b) = "Lookup for $(basetypeof(a)) of $(lookup(a)) and $(lookup(b)) do not match."
_ordermsg(a, b) = "Lookups do not all have the same order: $(order(a)), $(order(b))."
function _valtypenamemsg(a, b)
typenamea = Base.nameof(typeof(parent(a)))
typenameb = Base.nameof(typeof(parent(b)))
return "Lookup types for $(basetypeof(a)) do not match: $typenamea and $typenameb."
end

@noinline _dimsmismatchaction(err, a, b) = _failed_comparedims(err, _dimsmismatchmsg(a, b))
@noinline _valaction(err, a, b) = _failed_comparedims(err, _valmsg(a, b))
@noinline _dimsizeaction(err, a, b) = _failed_comparedims(err, _dimsizemsg(a, b))
@noinline _valtypeaction(err, a, b) = _failed_comparedims(err, _valtypemsg(a, b))
@noinline _orderaction(err, a, b) = _failed_comparedims(err, _ordermsg(a, b))
@noinline _valtypenameaction(err, a, b) = _failed_comparedims(err, _valtypenamemsg(a, b))

_failed_comparedims(w::Warn, msg_intro) = @warn string(msg_intro, msg(w))
_failed_comparedims(t::Throw, msg_intro) = throw(DimensionMismatch(string(msg_intro, msg(t))))
Expand Down Expand Up @@ -602,7 +608,7 @@ end
@inline _comparedims2(a::Dimension, b::AnonDim; kw...) = true
@inline _comparedims2(a::AnonDim, b::Dimension; kw...) = true
@inline function _comparedims2(a::Dimension, b::Dimension;
type=true, valtype=false, val=false, length=true, order=false,
type=true, valtypename = false, valtype=false, val=false, length=true, order=false,
ignore_length_one=false, msg
)
if type && basetypeof(a) != basetypeof(b)
Expand All @@ -616,6 +622,10 @@ end
isnothing(msg) || _orderaction(msg, a, b)
return false
end
if valtypename && Base.typename(typeof(parent(a))) != Base.typename(typeof(parent(b)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could introduce a function for this check and return false as fallback and return true for combinations that assume to be comparable

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like have this comparison dependent on the results of promote_first

isnothing(msg) || _valtypenameaction(msg, a, b)
return false
end
if valtype && typeof(parent(a)) != typeof(parent(b))
isnothing(msg) || _valtypeaction(msg, a, b)
return false
Expand Down
2 changes: 1 addition & 1 deletion src/array/broadcast.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ broadcast_options(A::BroadcastOptionsDimArray) = A.options
@inline function _comparedims_broadcast(A, dims...)
isstrict = _is_strict(A)
comparedims(dims...;
ignore_length_one=isstrict, order=isstrict, val=isstrict, length=false
ignore_length_one=isstrict, order=isstrict, val=isstrict, valtypename=isstrict, length=false
)
end

Expand Down
Loading