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

Corrections needed for 0.5.0 #11

Closed
ivanslapnicar opened this issue Sep 22, 2016 · 3 comments
Closed

Corrections needed for 0.5.0 #11

ivanslapnicar opened this issue Sep 22, 2016 · 3 comments

Comments

@ivanslapnicar
Copy link
Contributor

Jiahao,

greetings!
In 0.5.0 the package does not work. Attached is the small example notebook.
Can you please tell me how to correct it. I will than go through rest of the files
(and also implement the more appropriate indexing, as in the example).

Cheers, Ivan

SmallCheck.txt

@jiahao
Copy link
Contributor

jiahao commented Sep 28, 2016

Hi Ivan, good to hear from you!

The changes Ivan proposed are

#XXX Inefficient but works
getindex(H::Hankel, i, j) = getindex(full(H), i, j)
isassigned(H::Hankel, i, j) = isassigned(full(H), i, j)

to

getindex(H::Hankel, i, j) = H.c[i+j-1]
isassigned(H::Hankel,i,j) = isassigned(H.c,i+j-1)

which works on 0.4 but not 0.5; the latter throws an ambiguity error:

MethodError: isassigned(::Hankel{Int64}, ::Int64, ::Int64) is ambiguous. Candidates:
  isassigned(H::Hankel, i, j) at In[1]:13
  isassigned(a::AbstractArray, i::Int64...) at abstractarray.jl:185

I think all that is needed is to just add ::Int annotations to your new methods, like so:

getindex(H::Hankel, i::Int, j::Int) = H.c[i+j-1]
isassigned(H::Hankel, i::Int, j::Int) = isassigned(H.c,i+j-1)

Can you see if that works? If so, I'm happy to take a pull request for the fix.

@ivanslapnicar
Copy link
Contributor Author

Hi Jiahao,

problem solved, thanks a lot.

Now there is still smaller issue. The following code:

immutable Hankel{T} <: AbstractArray{T, 2}

c :: Vector{T}

end
Hankel{T}(c::Vector{T}) = length(c) % 2 == 1 ? Hankel{T}(c) : throw(ArgumentError(""))
...

returns a warning:

WARNING: Method definition (::Type{Main.Hankel})(Array{#T<:Any, 1}) in module Main at In[1]:4 overwritten at In[1]:4.

I dont know how to avoid this warning, but it is not so important.

I will go ahead and make some revisions in the package and do the push myself, now that you've given me the rights.

I hope this works.

Cheers, Ivan

Prof. dr. sc. Ivan Slapničar
Sveučilište u Splitu / University of Split
FESB
R. Boškovića 32
HR-21000 Split
Hrvatska / Croatia
Phone: + 385 21 305893
Ured/Office: B803


Šalje: Jiahao Chen [email protected]
Poslano: 28. rujna 2016. 18:46
Prima: jiahao/SpecialMatrices.jl
Kopija: Ivan Slapničar; Author
Predmet: Re: [jiahao/SpecialMatrices.jl] Corrections needed for 0.5.0 (#11)

Hi Ivan, good to hear from you!

The changes Ivan proposed are

#XXX Inefficient but works
getindex(H::Hankel, i, j) = getindex(full(H), i, j)
isassigned(H::Hankel, i, j) = isassigned(full(H), i, j)

to

getindex(H::Hankel, i, j) = H.c[i+j-1]
isassigned(H::Hankel,i,j) = isassigned(H.c,i+j-1)

which works on 0.4 but not 0.5; the latter throws an ambiguity error:

MethodError: isassigned(::Hankel{Int64}, ::Int64, ::Int64) is ambiguous. Candidates:
isassigned(H::Hankel, i, j) at In[1]:13
isassigned(a::AbstractArray, i::Int64...) at abstractarray.jl:185

I think all that is needed is to just add ::Int annotations to your new methods, like so:

getindex(H::Hankel, i::Int, j::Int) = H.c[i+j-1]
isassigned(H::Hankel, i::Int, j::Int) = isassigned(H.c,i+j-1)

Can you see if that works? If so, I'm happy to take a pull request for the fix.

You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com//issues/11#issuecomment-250225734, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AIRrsKEUZCeDOMeUTaXyQdi1KsHRDvWEks5qupnogaJpZM4KD-28.

@jiahao
Copy link
Contributor

jiahao commented Sep 29, 2016

I would consider the second issue a bug. Ref: JuliaLang/julia#14960

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

2 participants