Extend table to support arbitrary vector types

[amartinhuertas]

This PR should be reviewed after PR #309 is accepted

This PR solves issue #295

I have only the following concern/doubt w.r.t. the present PR. Should we keep the current interface for the indentity_table and empty_table methods? i.e,

Gridap.jl/src/Arrays/Tables.jl

Line 47 in cce0534

function identity_table(::Type{T},::Type{P},l::Integer) where {T,P}

... or should this interface be adapted somehow to the current implementation of Table?

[amartinhuertas]

Two tests are failing. See in particular:

https://travis-ci.com/github/gridap/Gridap.jl/jobs/357231183

The first of the two test fails in the first line of the following method:

function array_cache(a::LocalToGlobalArray)
  gids = testitem(a.lid_to_gid)
  T = eltype(a.gid_to_val)
  r = zeros(T,size(gids))
  c = CachedArray(r)
  cl = array_cache(a.lid_to_gid)
  (cl,c)
end

because a.lid_to_gid is such that typeof(first(a.lid_to_gid)) noteq eltype(a.lid_to_gid) (note that first() now returns a subarray). @fverdugo ... is this a strictly necessary precondition? why?

[fverdugo]

Hi @amartinhuertas. The extension of Table is not as obvious as it can seem. I am not 100% sure about how to do it properly to achieve performance and being consistent with the julia AbstractVector interface. That's why I have not done this yet...

The thing is that eltype has to be consistent with getindex (and also getindex! as we understand it now). If getindex returns subvectors or views then eltype has to be also subvector or view. However, getindex! can return plain julia arrays efficiently by taking advantage of the cache. So different eltype here... getinex! could return views, but I not sure if this is what we want since it can lead to a lot of fine-grained memory allocations if the compiler does not inline functions properly...

Perhaps we can allow getindex! to return a different type, but I am not sure if this is elegant...

[fverdugo]

Do not merge for the moment. See my comment above.

[amartinhuertas]

The thing is that eltype has to be consistent with getindex (and also getindex! as we understand it now). If getindex returns subvectors or views then eltype has to be also subvector or view.

For my understanding: Who imposes this constraint (i.e., eltype consistent with getindex)? Gridap.jl or AbstractVector/Julia?

[amartinhuertas]

Do not merge for the moment. See my comment above.

Can we at least partially merge this, leaving apart the following?

Allow setting values in a table using the standard julia interface of vector of vectors

Just an idea ...

[fverdugo]

For my understanding: Who imposes this constraint (i.e., eltype consistent with getindex)? Gridap.jl or AbstractVector/Julia?

Its the definition of eltype in Julia

[fverdugo]

we need to think how to do this properly before merging...

[fverdugo]

Perhaps it is better to discuss this face-to-face since it is not an obvious topic. The performance / design of Table has potential implications in the whole project. We need to be careful.

[amartinhuertas]

I found the following comment by one of the Julia developers, on an issue related to the topics we are discussing:

JuliaLang/julia#9586 (comment)

Accordingly to this comment, effectively the type returned by x[i] with i and Integer should be eltype(x)

[amartinhuertas]

Hi @fverdugo. I am positive that I need a general version of Table for GridapDistributed.jl (even we decided to put on hold this development for a while).

Recall that now we are going to leave on the hands of the user to select the type of the local vector for DistributedVectors.

If the user selects a Table, that's ok. This Table will be in the ordering of the APP, and not that of PETSc. Thus, I cannot pass the data array directly to PETSc.

In order to solve this, I need to create a new data array in the ordering of PETSc, and then build a new reindexed Table on top of it, which will be the one that the user will finally get after creating his DistributedVector.

So far so good, but I need the general Table in order to implement this. We can discard the part related to getindex/setindex, I only need Table generalization.

What do u think? Do u see another possible solution?

Thanks!

[fverdugo]

As far as I remember, this PR had two parts:

1. Allow general AbstractArray objects in the fields of a Table
2. Mutate a Table object as it would be an array of arrays

The part (1) can be done and merged since it is conceptually straight-forward (even though it will break some code downstream, but it should be easy to fix). However, part (2) is quite more difficult and we haven't found a (good) solution yet.

I assume that you only need part (1) right?

[amartinhuertas]

I assume that you only need part (1) right?

Yes, that's it. I am aware that this change will have an impact in Gridap.jl satellite packages as well (e.g., GridapEmbedded).

I am now going to eliminate (2) from this branch, and try to see whether the tests pass or not.

[amartinhuertas]

I am now going to eliminate (2) from this branch, and try to see whether the tests pass or not.

The tests passed! You can proceed with the code review (if you feel it is necessary). It was not rocket science.

[fverdugo]

Thanks for the changes!

[amartinhuertas]

Agreed with @fverdugo that the changes in this PR (branch extend_table_to_support_arbitrary_vector_types) are to be merged together with additional breaking changes that will go into v0.14 (release-0.14 branch). Thus, we can close and discard this PR.