JuliaLang / Compat.jl

Compatibility across Julia versions
Other
145 stars 117 forks source link

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

Closed sbromberger closed 9 years ago

sbromberger commented 9 years ago

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 commented 9 years ago

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

garrison commented 9 years ago

and the related julia-users thread

garrison commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

Perfect. Thank you very much!

sbromberger commented 9 years ago

Ok, just FYI, this is dev+4739.

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

stevengj commented 9 years ago

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 commented 9 years ago

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

stevengj commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

(...anyone?) :)

kmsquire commented 9 years ago

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 commented 9 years ago

@kmsquire my pleasure, but please note:

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

:)

sbromberger commented 9 years ago

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 commented 9 years ago

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 notifications@github.com 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 https://github.com/JuliaLang/Compat.jl/issues/105#issuecomment-117840134 .

kmsquire commented 9 years ago

110

sbromberger commented 9 years ago

Rockin'. Thanks.

sbromberger commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

On Jul 2, 2015, at 13:14, Kevin Squire notifications@github.com 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 https://github.com/JuliaLang/Compat.jl/issues/105#issuecomment-118114039.

kmsquire commented 9 years ago

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 commented 9 years ago

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

On Jul 2, 2015, at 13:14, Kevin Squire notifications@github.com 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 https://github.com/JuliaLang/Compat.jl/issues/105#issuecomment-118114039.

kmsquire commented 9 years ago

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 commented 9 years ago

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

https://github.com/JuliaGraphs/LightGraphs.jl/pull/70

On Jul 2, 2015, at 13:27, Kevin Squire notifications@github.com 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 https://github.com/JuliaLang/Compat.jl/issues/105#issuecomment-118116751.

sbromberger commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@kmsquire #111