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

Reorganize some hvcat functions #16741

Merged
merged 1 commit into from
Jun 7, 2016
Merged

Conversation

pabloferz
Copy link
Contributor

@pabloferz pabloferz commented Jun 3, 2016

This PR reorganize some hvcat functions and removes some duplicates of some cat operations that exist in both abstractarray.jl and array.jl fixes some problems that were introduced in commit e280a27. These cause some cat function to fallback to a generic (slow) version.

This should help with at least the rand_mat_mul in #16128.

@JeffBezanson
Copy link
Member

Thanks. However I think this might cause the definitions in sparse/sparsematrix.jl to be called, since they are more specific. I think that's why these were added.

@pabloferz
Copy link
Contributor Author

Yes, I was thinking about this. I'll see what I can do about it.

@pabloferz pabloferz changed the title Remove duplicates of some cat functions Reorganize some hvcat functions Jun 3, 2016
@pabloferz pabloferz force-pushed the pz/cat branch 2 times, most recently from d19994a to 72c5219 Compare June 3, 2016 17:43
@pkofod
Copy link
Contributor

pkofod commented Jun 3, 2016

Yes they were added specifically because the specialized methods would have been called. Maybe I used too specific types? Would it have sufficed with AbstractMatrix and AbstractSparseMatrix ?

@pabloferz
Copy link
Contributor Author

I don't think the building should give an ambiguous definition error. Is it a bug in inference or I am missing something obvious?

@pabloferz pabloferz force-pushed the pz/cat branch 2 times, most recently from fa17dcf to d1843c7 Compare June 6, 2016 14:34
@pabloferz
Copy link
Contributor Author

pabloferz commented Jun 6, 2016

Ok. @JeffBezanson I think this is ready and addresses the issue with rand_mat_mul in #16128

@kshyatt kshyatt added the arrays [a, r, r, a, y, s] label Jun 7, 2016
hvcat(rows::Tuple{Vararg{Int}}, xs::AbstractMatrix...) = typed_hvcat(promote_eltype(xs...), rows, xs...)
hvcat{T}(rows::Tuple{Vararg{Int}}, xs::AbstractMatrix{T}...) = typed_hvcat(T, rows, xs...)

function typed_hvcat{T}(TT::Type{T}, rows::Tuple{Vararg{Int}}, as::AbstractMatrix{T}...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires all the argument matrices to have the output type T as their element type already.

Also could you keep the code in place above to get a smaller diff?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. There was an apparently unrelated AV error.

@JeffBezanson
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants