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 Base.vect override #293

Merged
merged 1 commit into from
May 18, 2019
Merged

Remove Base.vect override #293

merged 1 commit into from
May 18, 2019

Conversation

ericphanson
Copy link
Collaborator

@ericphanson ericphanson commented May 13, 2019

Fixes #223.

I'm not sure if this change is OK or not. On the one hand, the tests pass. On the other hand, it must be here for a reason; maybe this change will break some obscure edge case. In the latter case though, it would be nice if we had a test to show we need this override (or even better, find a different resolution to such an edge case without overriding Base.vect).

I thought I'd make the pull request to open a discussion at least.

P.S. sorry for the barrage of pull requests; no rush to address these, I'm just trying to work through the issue backlog a bit.

@ararslan
Copy link
Contributor

That extension was included to ensure that the [A, B] syntax produces an HcatAtom. Is that still the case if these definitions are removed?

@ericphanson
Copy link
Collaborator Author

No, I don't think so:

julia> typeof([Variable(2), Variable(2)])
Array{Variable,1}

To be fair,

julia> typeof([rand(2), rand(2)])
Array{Array{Float64,1},1}

isn't an hcat either, is it?

@ericphanson
Copy link
Collaborator Author

ericphanson commented May 13, 2019

On this branch, we have

julia> [Variable(2), Variable(2)]
2-element Array{Variable,1}:
 Variable of
size: (2, 1)
sign: NoSign()
vexity: AffineVexity()
 Variable of
size: (2, 1)
sign: NoSign()
vexity: AffineVexity()

julia> [Variable(2) Variable(2)]
AbstractExpr with
head: hcat
size: (2, 2)
sign: NoSign()
vexity: AffineVexity()


julia> [Variable(2); Variable(2)]
AbstractExpr with
head: transpose
size: (4, 1)
sign: NoSign()
vexity: AffineVexity()

This behavior seems to match how usual vectors behave (e.g. substituting Variable for rand). Likewise for matrices:

julia> [Variable(2,3) Variable(2,3)]
AbstractExpr with
head: hcat
size: (2, 6)
sign: NoSign()
vexity: AffineVexity()


julia> [Variable(2,3); Variable(2,3)]
AbstractExpr with
head: transpose
size: (4, 3)
sign: NoSign()
vexity: AffineVexity()


julia> [Variable(2,3), Variable(2,3)]
2-element Array{Variable,1}:
 Variable of
size: (2, 3)
sign: NoSign()
vexity: AffineVexity()
 Variable of
size: (2, 3)
sign: NoSign()
vexity: AffineVexity()

So we do have hcat and vcat working when you separate with a space or semicolon, as with usual vectors or matrices, and it creates a list when separating with a comma. To me, this seems like natural behavior. Why do we want the comma separation to result in hcat too?

@ericphanson
Copy link
Collaborator Author

ericphanson commented May 13, 2019

If my understanding is right, I think the proposed changed here is analogous to JuliaLang/julia#8599 in Base, which seems to have introduced Base.vect, leading to #145 and these lines in Convex in the first place. So if I'm understanding it right, the proposed change here is to match the behavior in Base instead of overriding it for Variable types. This is thus a breaking change to Convex, to match the associated one in Base.

But I think I am missing some context, because I'd think the resolution in #145 would have been to match base behavior if that had been an acceptable option at that time. Perhaps now it is an acceptable change? Or maybe those reasons that kept it from being a solution to #145 are still present.

@ericphanson
Copy link
Collaborator Author

It seems like this change also fixes #282 (see #282 (comment)).

@ararslan
Copy link
Contributor

Tbh I don't really know what the right thing to do here is, but your explanation with regards to the change in Base is compelling. Let's go for it and see what happens.

@ararslan ararslan merged commit 6bccf6a into jump-dev:master May 18, 2019
@ericphanson
Copy link
Collaborator Author

Okay, exciting!

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.

Array of Variable is Unexpected behavior
2 participants