Periodic BCs

[fverdugo]

Draft of the changes in code related with CartesianDescriptor to include periodic BCs

struct CartesianDescriptor{D,T,F<:Function} <: GridapType
  origin::Point{D,T}
  sizes::NTuple{D,T}
  partition::NTuple{D,Int}
  isperiodic::NTuple{D,Bool}
  map::F
  @doc """
      CartesianDescriptor(
        origin::Point{D}, 
        sizes::NTuple{D}, 
        partition,
        map::Function=identity,
        isperiodic::NTuple{D,Bool}=tfill(false,Val{D}())) where D
  `partition` is a 1D indexable collection of arbitrary type.
  """
  function CartesianDescriptor(
        origin::Point{D}, 
        sizes::NTuple{D}, 
        partition,
        map::Function=identity,
        isperiodic::NTuple{D,Bool}=tfill(false,Val{D}())) where D

    T = eltype(sizes)
    F = typeof(map)
    new{D,T,F}(origin,sizes,Tuple(partition),map,isperiodic)
  end

end

function CartesianDescriptor(
        origin::Point{D}, 
        sizes::NTuple{D}, 
        partition;
        map::Function=identity,
        isperiodic::NTuple{D,Bool}=tfill(false,Val{D}())) where D
end


function CartesianGrid(args...;kwargs...)
  desc = CartesianDescriptor(args...;kwargs...)
  CartesianGrid(desc)
end

# Idem for CartesianDiscreteModel

[jesusbonilla]

We decided to implement two different methods for every constructor of the CartesianDescriptor, one with map and isperiodic as optional arguments and another with them as keyword arguments. If we do so, and you call the function without any optional argument, then it is ambiguous which method should be used. Actually, the following warning is thrown at precompilation time:

WARNING: Method definition (::Type{Gridap.Geometry.CartesianDescriptor{D, T, F} where
F<:Function where T where D})(Gridap.TensorValues.MultiValue{Tuple{D}, T, 1, D} where T, 
Tuple{Vararg{T, D}} where T, Any) where {D} in module Geometry at 
.../src/Geometry/CartesianGrids.jl:37 overwritten at .../src/Geometry/CartesianGrids.jl:68.

  ** incremental compilation may be fatally broken for this module **

To keep backward compatibility only isperiodic should be a keyword argument, but it might be odd to have map as an optional and isperiodic as a keyword argument.

[fverdugo]

Yes you are right.

I would do the following:

1. Make all optional positional arguments mandatory.
2. Use kw-args when you want optional arguments.
3. Then, implement specific extra functions where needed to keep backwards compatibility

Does it solve the problem?

[fverdugo]

We can even mark as "Deprecated" functions in step 3 and remove them in the next breaking release.

[santiagobadia]

As @fverdugo knows, I lobby kwargs for constructors of non-obvious objects that are exposed to the user. They provide extensibility without affecting backward compatibility. I would not use optional arguments in these constructors to be used by users, it is a mess, and it affects backward compatibility when you want extensions.

[jesusbonilla]

Ok, done! Thanks for the tips.

[fverdugo]

closed via PR #266