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

Reorganization of linalg.jl in separate files #750

Merged
merged 3 commits into from
Apr 24, 2012

Conversation

carlobaldassi
Copy link
Member

This follows this discussion. Everything seems to be functional (all tests pass), and I also started adding some more sparse functions, so it could be merged already if it looks fine. Of course it's very preliminary (e.g. the interface in linalg is mostly "specified" by commented function prototypes, but that's supposed to change with time, and anyway it's no worse than current situation).

(Some very minor points still need fixing/settling, I left commented XXX's as placeholders to remind where the problematic parts are.)

linalg.jl is now mostly an empty shell; all functions
were moved to linalg_dense.jl and specialized for Arrays.
Other AbstractArray implementations are supposed to provide
linear algebra interface in their own linalg_<type>.jl file.
(Currently available linalg functions for sparse and bitarrays
were moved in their largely incomplete files.)

Some issues in linalg.jl, marked by XXX comments, still need to be
fixed.
@ViralBShah
Copy link
Member

Browsing through the diff, looks ok. Leaving it around for a bit longer for others to take a look, before merging.

@JeffBezanson
Copy link
Member

Basically good. I would only make a couple changes, for the cases where code can literally be copied and pasted with just the signature changed from Matrix to BitMatrix, such as norm. Anything like that should be defined for AbstractMatrix.

I can go in and insert another type above Array. DenseArray is actually not quite right since a DArray is also dense. Maybe UniformArray?

@pao
Copy link
Member

pao commented Apr 22, 2012

"Uniform" in terms of memory access? I'm not sure that's obvious (I had to think about it for a minute, and I might still be wrong.) But it's not a strong objection.

@ViralBShah
Copy link
Member

What does Uniform mean? Not sure I like that name.

-viral

On 22-Apr-2012, at 4:12 AM, JeffBezansonreply@reply.github.com wrote:

Basically good. I would only make a couple changes, for the cases where code can literally be copied and pasted with just the signature changed from Matrix to BitMatrix, such as norm. Anything like that should be defined for AbstractMatrix.

I can go in and insert another type above Array. DenseArray is actually not quite right since a DArray is also dense. Maybe UniformArray?


Reply to this email directly or view it on GitHub:
#750 (comment)

@JeffBezanson
Copy link
Member

Well there are two main issues: density, and uniformity of access time. DArray is dense, but has wildly nonuniform access time. Sparse is sparse, but has fairly uniform access time (at least compared to DArray). What we want is a type that captures "arrays that work with the definitions carlo had to copy and paste into bitarray.jl".

@ViralBShah
Copy link
Member

How about UTArray, for Uniform time access array. We could use a better name if we come up with it later. Is this the only reason this merge request is held? Should this refactoring be done before this pull request is merged?

@JeffBezanson
Copy link
Member

No, we don't have to do the UTArray change for this particular pull request.
Just see my previous comment about adding back the Abstract in cases where code was duplicated by this patch.

@carlobaldassi
Copy link
Member Author

I have removed duplicated code. BitArrays will have those functions again when there will be the intermediate level between AbstractArray and Array, so I think this could be merged now.

Speaking of the intermediate level name: what about ContiguousArray? It seems to express both density and locality, doesn't it?

Also, while I'm at it: what about making StridedArray a part of the arrays hierarchy as well, rather than a Union?

AbstractArray
ContiguousArray <: AbstractArray  # or UTArray or whatever
StridedArray <: ContiguousArray
Array <: StridedArray
SubArray <: StridedArray
BitArray <: ContiguousArray

(At some point I was thinking about BitArray <: StridedArray but it doesn't seem to fit really.)

@ViralBShah
Copy link
Member

Isn't even SparseMatrixCSC Contiguous? It is better than UniformArray, but still not good enough, IMO. How about UniformAccessArray?

I like the hierarchy.

-viral

On 23-Apr-2012, at 3:39 PM, Carlo Baldassi wrote:

I have removed duplicated code. BitArrays will have those functions again when there will be the intermediate level between AbstractArray and Array, so I think this could be merged now.

Speaking of the intermediate level name: what about ContiguousArray? It seems to express both density and locality, doesn't it?

Also, while I'm at it: what about making StridedArray a part of the arrays hierarchy as well, rather than a Union?

AbstractArray
ContiguousArray <: AbstractArray  # or UTArray or whatever
StridedArray <: ContiguousArray
Array <: StridedArray
SubArray <: StridedArray
BitArray <: ContiguousArray

(At some point I was thinking about BitArray <: StridedArray but it doesn't seem to fit really.)


Reply to this email directly or view it on GitHub:
#750 (comment)

@carlobaldassi
Copy link
Member Author

I was intending "contiguous" in the sense of strides-aware-contiguous mapping from indices to memory. But yes, it seems UniformAccess may be better, and more abstract.

@StefanKarpinski
Copy link
Member

It seems to me that the right definition of contiguous might just be "dense" — i.e. stores all values rather than having defaults where no value is actually stored.

JeffBezanson added a commit that referenced this pull request Apr 24, 2012
Reorganization of linalg.jl in separate files
@JeffBezanson JeffBezanson merged commit bfd7bcc into JuliaLang:master Apr 24, 2012
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

Successfully merging this pull request may close these issues.

5 participants