Name inconsistency for the vector of spaces between MultiFieldFESpace and DistributedMultiFieldFESpace

[oriolcg]

trafficstars

Hi @amartinhuertas , @fverdugo,

In Gridap, the member variable that contains the vector of SingleFieldFESpace in the MultiFieldFESpace is called spaces, while in DistributedMultiFieldFESpace is called field_fe_space.

In GridapODEs we are accessing this variable as U.spaces, which results in an error when U is a DistributedMultiFieldFESpace. Do you thing the name can be changed to match the definition of MultiFieldFESpace? Otherwise, we'll have to define a getter for both types:

get_spaces(U::MultiFieldFESpace) = U.spaces
get_spaces(U::DistributedMultiFieldFESpace) = U.field_fe_space

What would be the best solution?

[amartinhuertas]

Hi @oriolcg,

thanks for raising this issue. I think it is best to homogeneize member variable names among counterpart types in Gridap and GridapDistributed (e.g., the code becomes more readable, apart from the fact that you do not have to define two variants of get_spaces). On the other hand, I think field_fe_space is a more intention revealing name than spaces. I would use the former, may be with a trailing s, i.e., field_fe_spaces for both packages. What do you think, @oriolcg, @fverdugo?

[amartinhuertas]

BTW ... shouldn't it be more appropriate that the method get_spaces is defined on Gridap.jl/GridapDistributed.jl (instead of GridapODEs.jl) as a method belonging to the interface of Multifield FE spaces, no matter whether they are serial or distributed?

[fverdugo]

Hi @oriolcg @amartinhuertas, I would use indexing/iteration just like for the FEFunction:

V = MultiFieldFESpace([V1,V2,V3])
V1,V2,V3 = V
V2 = V[2]

[amartinhuertas]

I would use indexing/iteration just like for the FEFunction:

Ok, agreed. This is the best option. You would not even require to define get_spaces. Still there is a name consistency issue, that we may solve or not.

[fverdugo]

I think field_fe_space is a more intention revealing name than spaces. I would use the former, may be with a trailing s, i.e., field_fe_spaces for both packages. What do you think, @oriolcg, @fverdugo?

If you really want to rename one, I would rename the one in Gridap to match field_fe_space. But I think that we can treat these fields as private, just as an implementation detail.

[oriolcg]

Ok, Is see. In GridapODEs would this work? Instead of doing:

for single_U in U.spaces
  ...
end

Just do:

for single_U in U
  ...
end

If this works I'll do that and change the name in both libraries to field_fe_spaces, and we don't need the getter.

[fverdugo]

If this works I'll do that and change the name in both libraries to field_fe_spaces, and we don't need the getter.

If it is not needed I would not rename the existing fields. Just implement the indexing/iteration by looking the source code of FEFunction.

Perhaps, when working on Gridap 0.18 we can further discuss about the renaming, but for the moment (Gridap 0.17) I'd rather not introduce potentially breaking changes.

[oriolcg]

The indexing/iteration functions are already implemented for MultiFieldFESpace and DistributedMultiFieldFESpace. I will just need the length function to do zipped loops of the type:

for (Uti,Ui) in zip(Ut,U)
 ...
end

With Ut and U (Transient/Distributed)MultiFieldFESpace

[fverdugo]

The indexing/iteration functions are already implemented

That's great! I didn't remember!

If U[2] is available, I think it makes sense to also have length(U).