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

@compat for (e.g.) Vector{Int}()? #105

Closed
sbromberger opened this issue Jun 18, 2015 · 32 comments
Closed

@compat for (e.g.) Vector{Int}()? #105

sbromberger opened this issue Jun 18, 2015 · 32 comments

Comments

@sbromberger
Copy link
Contributor

Hi,

Currently in 0.3.x

julia> q = Vector{Int}()
ERROR: type cannot be constructed

Any way to get this working in 0.3?

@garrison
Copy link
Member

see also JuliaLang/julia#11154, where the Vector{Int} constructor was added.

@garrison
Copy link
Member

and the related julia-users thread

@garrison
Copy link
Member

In any event, I have been planning to add Compat support for these constructors but have been fairly busy lately, and I expect to be busy/unresponsive the next two weeks. Feel free to beat me to it. Some documentation for these constructors should also be added to julia once the julia-users thread pans out.

@sbromberger
Copy link
Contributor Author

@garrison thanks. I might take a shot at it next week but I doubt my attempt will be as good as yours would be.

I think the julia-users thread has run its course, and unfortunately (for me, anyway) there hasn't been a declaration. I'm still left wondering what the go-forward, preferred method of constructing an empty vector is / will be. If it's Vector{Int}(), then I can feel comfortable trying to get this into Compat. If, however, this is NOT the best way to do it, then I'd much rather try to get the preferred method in.

I hope this rambling makes a little bit of sense.

@StefanKarpinski
Copy link
Member

We should definitely have this in Compat. If you want a really future-proof way to construct an empty vector of a given type, Array{Int}(0) is the safest possible thing – I can't see that going away.

@sbromberger
Copy link
Contributor Author

@StefanKarpinski thanks very much for your comment. If I'm interpreting it correctly, it means that Vector{T}() is the go-forward Julian way to create an empty vector, and that the other methods (T[], ...) are "ok but not preferred"?

(or, on second read, does it mean that Vector{T}() is not as preferred as Array{T}(0)? Or is Vector merely syntactic sugar that can be relied upon?)

@StefanKarpinski
Copy link
Member

No, I'm just saying that if you want to be maximally sure that you don't ever have to change your code, use Array{T}(d1, d2, ...) – that form is not going anywhere. The Vector{T}(d1) thing is very nearly as safe, although it's slightly odd that the number of dimensions and the Vector name are redundant. The Vector{T}() form with no arguments strikes me as a little fishy – why is zero the default dimension size? Similarly for Matrix{T}() giving a 0x0 matrix of element type T. You can certainly use the T[] syntax for now, and who knows, it might stay, but the indexing into a type syntax pun is kind of unfortunate so I suspect we might come up with something better at some point.

In short, if you're ok with potentially changing your code in the future when you get deprecation warnings, then use whatever syntax you prefer. If you really want to avoid that, use Array{T}(0) for T[].

@sbromberger
Copy link
Contributor Author

Perfect. Thank you very much!

@sbromberger
Copy link
Contributor Author

Ok, just FYI, this is dev+4739.

... and I have absolutely no idea how to do this without call :)

@stevengj
Copy link
Member

You would write a macro, so that @compat Vector{Int}() gets left as-is in 0.4 and in older versions gets transformed to Array(Int). It's no problem to do this via a macro because Vector{Int}() still parses in older versions, even if it cannot be compiled.

@sbromberger
Copy link
Contributor Author

Thanks. I've never written a macro before. Perhaps this is a good opportunity to figure it out.

@stevengj
Copy link
Member

See the existing macro compat in the Compat package; it already does several transformations of this sort, so you would just be adding one more.

@sbromberger
Copy link
Contributor Author

Does this work for the array rewrite? (This is my first attempt at metaprogramming; my tests seem to work but there may be edge cases).

I don't know where to put this, in any case.

Vector appears to be a :ref instead of a :call so there will likely need to be another function.

function rewrite_arr(ex)
    f = ex.args[1]
    if isexpr(f, :curly)
        head = ex.head
        args = [f.args[1:2], ex.args[2:end]]
        return Expr(head, args...)
    else
        return ex
    end
end

@sbromberger
Copy link
Contributor Author

(...anyone?) :)

@kmsquire
Copy link
Member

Thanks for doing this, and for bumping. If no one else gets to it first, I'll look at this tonight. (I also need it!)

@sbromberger
Copy link
Contributor Author

@kmsquire my pleasure, but please note:

  1. My code's probably wrong; and
  2. This doesn't fix Vector{Int}().

:)

@sbromberger
Copy link
Contributor Author

Hi all -

I'm sorry to be such a pain about this, but it's holding up a fairly big commit I have in LightGraphs. I could work around it by using Int[], but I'd rather do the right thing than have to revisit it later.

@kmsquire
Copy link
Member

kmsquire commented Jul 1, 2015

Sorry, wasn't able to get to it the other night, but I can do so now.

On Wed, Jul 1, 2015 at 3:19 PM, Seth Bromberger [email protected]
wrote:

Hi all -

I'm sorry to be such a pain about this, but it's holding up a fairly big
commit I have in LightGraphs. I could work around it by using Int[], but
I'd rather do the right thing than have to revisit it later.


Reply to this email directly or view it on GitHub
#105 (comment)
.

@kmsquire
Copy link
Member

kmsquire commented Jul 2, 2015

#110

@sbromberger
Copy link
Contributor Author

Rockin'. Thanks.

@sbromberger
Copy link
Contributor Author

Not sure this is working properly. I'm getting an error in nightly with

type TarjanVisitor <: AbstractGraphVisitor
    stack::Vector{Int}
    lowlink::Vector{Int}
    index::Vector{Int}
    components::Vector{Vector{Int}}
end

TarjanVisitor(n::Int) = TarjanVisitor(
    @compat Vector{Int}(),
    @compat Vector{Int}(),
    zeros(Int, n),
    @compat Vector{Vector{Int}}()
)

:

ERROR: LoadError: LoadError: MethodError: `convert` has no method matching convert(::Type{LightGraphs.TarjanVisitor}, ::Tuple{Array{Int64,1},Tuple{Array{Int64,1},Array{Int64,1},Array{Array{Int64,1},1}}})

@kmsquire
Copy link
Member

kmsquire commented Jul 2, 2015

Is the error in Julia v0.4, or Julia v0.3? In Julia v0.4, the code I added should be a no-op, and your code works fine for me on an out-of-date (21 days old) master. I haven't tested on Julia v0.3

@sbromberger
Copy link
Contributor Author

Fails on 0.4 on a 35-day-old master.

On Jul 2, 2015, at 13:14, Kevin Squire [email protected] wrote:

Is the error in Julia v0.4, or Julia v0.3? In Julia v0.4, the code I added
should be a no-op, and your code works fine for me on an out-of-date (21
days old) master. I haven't tested on Julia v0.3


Reply to this email directly or view it on GitHub
#105 (comment).

@kmsquire
Copy link
Member

kmsquire commented Jul 2, 2015

Can you repeat the commands below and show the output?

$ julia
               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "help()" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.4.0-dev+5307 (2015-06-10 22:36 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit 5035c5c* (21 days old master)
|__/                   |  x86_64-apple-darwin14.3.0

julia> using LightGraphs

julia> using Compat

julia> type TarjanVisitor <: AbstractGraphVisitor
           stack::Vector{Int}
           lowlink::Vector{Int}
           index::Vector{Int}
           components::Vector{Vector{Int}}
       end

julia> TarjanVisitor(n::Int) = TarjanVisitor(
           @compat Vector{Int}(),
           @compat Vector{Int}(),
           zeros(Int, n),
           @compat Vector{Vector{Int}}()
       )
TarjanVisitor

julia> @compat TarjanVisitor(n::Int) = TarjanVisitor(
           Vector{Int}(),
           Vector{Int}(),
           zeros(Int, n),
           Vector{Vector{Int}}()
       )
TarjanVisitor

julia> macroexpand(:(@compat TarjanVisitor(n::Int) = TarjanVisitor(
           Vector{Int}(),
           Vector{Int}(),
           zeros(Int, n),
           Vector{Vector{Int}}()
       )))
:(TarjanVisitor(n::Int) = begin  # none, line 1:
            TarjanVisitor(Vector{Int}(),Vector{Int}(),zeros(Int,n),Vector{Vector{Int}}())
        end)

@sbromberger
Copy link
Contributor Author

Sorry. The failure was on 0.4 nightly (Travis).

On Jul 2, 2015, at 13:14, Kevin Squire [email protected] wrote:

Is the error in Julia v0.4, or Julia v0.3? In Julia v0.4, the code I added
should be a no-op, and your code works fine for me on an out-of-date (21
days old) master. I haven't tested on Julia v0.3


Reply to this email directly or view it on GitHub
#105 (comment).

@kmsquire
Copy link
Member

kmsquire commented Jul 2, 2015

Okay. When you get the chance, can you point to the Travis output with problems, and/or provide explicit instructions on how to reproduce? Thanks!

@sbromberger
Copy link
Contributor Author

Can't get Travis atm but here's the github pr. It's the most recent failure:

sbromberger/LightGraphs.jl#70

On Jul 2, 2015, at 13:27, Kevin Squire [email protected] wrote:

Okay. When you get the chance, can you point to the Travis output with
problems, and/or provide explicit instructions on how to reproduce? Thanks!


Reply to this email directly or view it on GitHub
#105 (comment).

@sbromberger
Copy link
Contributor Author

@kmsquire rerunning Travis now... the build is here: https://travis-ci.org/JuliaGraphs/LightGraphs.jl/builds/69349027

It's weird because it looks like the @compat Vector{Int}() is turning into ::Tuple{Array{Int64,1} (see line 223 from the nightly output).

PS: output from the above:

seth@schroeder:~/dev/julia/wip/LightGraphs.jl$ julia
               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "help()" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.4.0-dev+5008 (2015-05-26 16:08 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit 0855ec9 (37 days old master)
|__/                   |  x86_64-apple-darwin14.3.0

julia> using LightGraphs
using ComWarning: could not import Base.serialize_type into DataStructures
pat

julia> using Compat

julia> type TarjanVisitor <: AbstractGraphVisitor
                  stack::Vector{Int}
                  lowlink::Vector{Int}
                  index::Vector{Int}
                  components::Vector{Vector{Int}}
              end

julia> TarjanVisitor(n::Int) = TarjanVisitor(
                  @compat Vector{Int}(),
                  @compat Vector{Int}(),
                  zeros(Int, n),
                  @compat Vector{Vector{Int}}()
       )
TarjanVisitor

julia> @compat TarjanVisitor(n::Int) = TarjanVisitor(
                  Vector{Int}(),
                  Vector{Int}(),
                  zeros(Int, n),
                  Vector{Vector{Int}}()
              )
TarjanVisitor

julia> macroexpand(:(@compat TarjanVisitor(n::Int) = TarjanVisitor(
                  Vector{Int}(),
                  Vector{Int}(),
                  zeros(Int, n),
                  Vector{Vector{Int}}()
              )))
:(TarjanVisitor(n::Int) = begin  # none, line 1:
            TarjanVisitor(Vector{Int}(),Vector{Int}(),zeros(Int,n),Vector{Vector{Int}}())
        end)

@sbromberger
Copy link
Contributor Author

Moving the @compat to the beginning of the constructor made it all work:

https://travis-ci.org/JuliaGraphs/LightGraphs.jl/jobs/69353258

@compat TarjanVisitor(n::Int) = TarjanVisitor(
    Vector{Int}(),
    Vector{Int}(),
    zeros(Int, n),
    Vector{Vector{Int}}()
)

(Also working on 0.3.)

@sbromberger
Copy link
Contributor Author

Also see the differences here:

julia> macroexpand(:(TarjanVisitor(n::Int) = TarjanVisitor(
                         @compat Vector{Int}(),
                         @compat Vector{Int}(),
                         zeros(Int, n),
                         @compat Vector{Vector{Int}}()
              )))
:(TarjanVisitor(n::Int) = begin  # none, line 1:
            TarjanVisitor((Vector{Int}(),(Vector{Int}(),zeros(Int,n),Vector{Vector{Int}}())))
        end)

julia> macroexpand(:(@compat TarjanVisitor(n::Int) = TarjanVisitor(
                         Vector{Int}(),
                         Vector{Int}(),
                         zeros(Int, n),
                         Vector{Vector{Int}}()
                     )))
:(TarjanVisitor(n::Int) = begin  # none, line 1:
            TarjanVisitor(Vector{Int}(),Vector{Int}(),zeros(Int,n),Vector{Vector{Int}}())
        end)

@kmsquire
Copy link
Member

kmsquire commented Jul 2, 2015

Ok, thanks. Can you open up a separate issue with that output? It should be the same, but I'm not sure that this problem would be restricted to this particular change.

@sbromberger
Copy link
Contributor Author

@kmsquire #111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants