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.

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

remove piracy in hcat, vcat, hvcat #513

Closed
wants to merge 2 commits into from
Closed

Conversation

ericphanson
Copy link
Collaborator

closes #512

@ericphanson ericphanson changed the title remove piracy remove piracy in hcat, vcat, hvcat Oct 24, 2023
@odow
Copy link
Member

odow commented Oct 24, 2023

I don't know that I like this approach.

Wouldn't it be better to force Convex.hcat and Convex.vcat?

Alternatively, this might work:

Base.hcat(args::AbstractExpr...) = HcatAtom(args...)

function Base.hcat(args::AbstractExprOrValue...)
    if all(Base.Fix2(isa, Value), args)
        return Base.cat(args..., dims = Val(2))
    end
    return HcatAtom(map(arg -> convert(AbstractExpr, arg), args)...)
end

@ericphanson
Copy link
Collaborator Author

ericphanson commented Oct 24, 2023

I agree, this is unsatisfying. However: Convex.hcat doesn't allow use of the [A B] implicit hcat syntax which is often used in these problems. And I think

function Base.hcat(args::AbstractExprOrValue...)
    if all(Base.Fix2(isa, Value), args)
        return Base.cat(args..., dims = Val(2))
    end
    return HcatAtom(map(arg -> convert(AbstractExpr, arg), args)...)
end

is the same piracy as we are doing now, no? It's the method signature that matters for that, not what happens inside

edit: or are you saying removing the hcat(args::Value...) = Base.cat(args..., dims = Val(2)) method might be sufficient to fix precomilation?

@KristofferC
Copy link

Just FYI, these type of combinatorial method definition can lead to bad load times for the package (JuliaDiff/ReverseDiff.jl#226).

@ericphanson
Copy link
Collaborator Author

ericphanson commented Oct 24, 2023

Yeah, I noticed that when I initially chose n=32 which never finished precompiling 😂. Do you have any ideas for how this can be improved?

I think the "right" fix is to be able to express "Varags{Union{Value,AbstractExpr}} but at least 1 AbstractExpr" in the type system, but I am guessing that isn't so easy to do. (I guess JuliaLang/julia#39878 is the issue for that).

with 57 methods for the three functions, load times "feel" ok to me, but increasing n blows up the number of methods very quickly.

@odow
Copy link
Member

odow commented Oct 24, 2023

is the same piracy as we are doing now, no?

Sort of, but to a lesser extent. Since AbstractExprOrValue is basically Union{Number,AbstractVector,AbstractExpr}, it should take lower precedence than the Union{Number,AbstractVector} method in Base.

@odow odow deleted the eph/no-piracy branch January 10, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fixing type piracy is becoming more urgent
3 participants