Towards a generalized implementation of RT FEs that does not assume an oriented global mesh. #579

amartinhuertas · 2021-04-14T10:38:37Z

Pull request that stemmed from the discussion in #577

Tests are not passing. Spent quite a lot of time debugging the following simple case, and I do not see where is the source of the error.

using Test
using Gridap.Geometry
using Gridap.ReferenceFEs
using Gridap.FESpaces
using Gridap.CellData
using Gridap.TensorValues
using Gridap.Fields
using Gridap.Io


order = 0

reffe = ReferenceFE(TRI,raviart_thomas,order)

domain =(0,1,0,1)
partition = (1,1)
model = simplexify(CartesianDiscreteModel(domain,partition))

V = FESpace(model,reffe,conformity=DivConformity())

v(x) = VectorValue(-0.5*x[1]+1.0,-0.5*x[2])
vh = interpolate(v,V)

e = v - vh

Ω = Triangulation(model)
dΩ = Measure(Ω,2*order)

el2 = sqrt(sum( ∫( e⋅e )*dΩ ))
@test el2 < 1.0e-10

@santiagobadia ... we should perhaps discuss what I did in this PR so that we can try to find the cause of the error.

In any case, I still do not understand two things:

How we ensure that the outward unit normals in physical space are actually unit normals? I do not see where they are scaled after applying the inverse transpose of the Jacobian to the unit normals in reference space.
Before my mods, the code was also using the signed determinant of the Jacobian for the interior moments. I am not longer using the signed determinant of the Jacobian, but the absolute value. This would affect to order>1 RT FEs. see https://github.com/gridap/Gridap.jl/pull/579/files#diff-ba506509a89b9ff975762ef98f39e3dd9a7d0ef2b6fdb3b962576fd48abdd161R115

that does not assume an oriented global mesh. Not passing tests. To investigate.

santiagobadia · 2021-04-14T11:23:49Z

Let us talk tomorrow at Monash.

global DoFs are transformed into local DoFs

amartinhuertas · 2021-04-23T10:21:18Z

A follow-up on this PR.

I have simplified the test above, using v(x) = VectorValue(1,2) instead of v(x) = VectorValue(-0.5*x[1]+1.0,-0.5*x[2])

When I restrict the the interpolated function to cell number 2, I still get the function with the sign flipped, i.e., [-1 -2].

I have done all computations by hand (see attachment), and double-checked that they match what the program is doing.

Thus, it might happen that I have missunderstood something in the implementation of RT FEs for arbitrarily oriented meshes

@santiagobadia ... do you know what might be wrong in the notes?

debugging_rt_fes.pdf

amartinhuertas · 2021-04-23T22:40:40Z

@santiagobadia ... Can it be that the signed determinant of the Jacobian has to be used for the Piola map if and only if you also use it when transforming the differential integral measure? I just thought that it makes sense as the Piola map somehow can be derived from the transformation of domain of integration.

In any case, not sure about this. If this was true, it would mean that, e.g., fenics is not using the absolute value of the determinant of the Jacobian when transforming the Domain of integration .... How would you solve the missorientation issue when integrating?
Perhaps with an extra sign flip which comes from info of the mesh?

…g_rt_fes_for_arbitrarily_oriented_meshes

(documentation added to RaviartThomasRefFEs.jl) Two key ingredients in the implementation of this type of ReferenceFE are the get_shapefuns(model,cell_reffes) and get_dof_basis(mode,cell_reffes) overloads. These are written such that, for each cell K, they return the shape functions and dof values in the *global* RT space. For a dof owned by a face which is shared by two cells, there is a master and a slave cell. The slave cell first computes the shape functions and dof values using local-to-cell data structures, but then flips the sign of both in order to get their corresponding counterparts in the **global** RT space. As as result we have the following: * When we interpolate a function into the global FE space, and we perform the cell-wise DoF values to global DoF values gather operation, we can either extract the global DoF value from the master or slave cell without worrying about the sign. * When we evaluate a global FE function, and we perform the global DoF values to cell-wise DoF values scatter operation, we don't have to worry about the sign either. On the slave cell, we will have both the sign of the DoF value, and the sign of the shape function corresponding to the global DoF. * We do NOT have to use the signed determinant, but its absolute value, in the Piola Map.

amartinhuertas · 2021-04-27T03:55:41Z

@fverdugo @santiagobadia ... FYI ...

I followed a different approach for generalizing RT FEs for arbitrarily oriented meshes to the one that I was trying to follow so far. See description of the following commit 3b6b880 (also added to the source code).

I think that once the tests pass, the PR is ready to review.

As a comment, I had to put the following code:

https://github.com/gridap/Gridap.jl/pull/579/files#diff-4a26cbf651e1f78f9b883733fcaf89ad35a3fabaa2e4938c669e5fd333d3ff63R144

in the ConformingFESpaces.jl source file. However, conceptually I guess tha this code should go to the RaviartThomasRefFEs.jl file. I could not put it in the former file because of a module cycle (Reffe cannot depend on Model).
Is there any known workaround to this in Julia? Did you have to face similar issues in the past?

amartinhuertas · 2021-04-27T04:02:59Z

Before my mods, the code was also using the signed determinant of the Jacobian for the interior moments. I am not longer using the signed determinant of the Jacobian, but the absolute value. This would affect to order>1 RT FEs. see https://github.com/gridap/Gridap.jl/pull/579/files#diff-ba506509a89b9ff975762ef98f39e3dd9a7d0ef2b6fdb3b962576fd48abdd161R115

Ok. I think this is no longer a problem. The code was using the signed determinant of the Jacobian both for the Piola Map and the calculation of the interior moments. Now we use the absolute value of the determinant both for the Piola Map and the calculation of interior moments. Thus, the outcome should be same, the tests with high order FEs should pass now.

codecov-commenter · 2021-04-27T04:12:08Z

Codecov Report

Merging #579 (571462f) into master (ab2a503) will increase coverage by 0.29%.
The diff coverage is 85.26%.

@@            Coverage Diff             @@
##           master     #579      +/-   ##
==========================================
+ Coverage   87.55%   87.84%   +0.29%     
==========================================
  Files         130      132       +2     
  Lines       13619    14183     +564     
==========================================
+ Hits        11924    12459     +535     
- Misses       1695     1724      +29

Impacted Files	Coverage Δ
src/Exports.jl	`0.00% <ø> (ø)`
src/FESpaces/FESolvers.jl	`88.23% <ø> (ø)`
src/ReferenceFEs/ReferenceFEInterfaces.jl	`66.90% <ø> (+0.43%)`	⬆️
src/FESpaces/FESpaceInterface.jl	`77.10% <26.66%> (-3.39%)`	⬇️
src/FESpaces/FESpacesWithConstantFixed.jl	`73.75% <50.00%> (ø)`
src/ReferenceFEs/RaviartThomasRefFEs.jl	`90.90% <54.54%> (+0.13%)`	⬆️
src/FESpaces/ZeroMeanFESpaces.jl	`84.61% <66.66%> (ø)`
src/FESpaces/CLagrangianFESpaces.jl	`94.28% <71.42%> (+0.05%)`	⬆️
src/FESpaces/ConformingFESpaces.jl	`92.37% <71.42%> (-1.75%)`	⬇️
src/FESpaces/DivConformingFESpaces.jl	`95.52% <95.52%> (ø)`
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab2a503...571462f. Read the comment docs.

amartinhuertas · 2021-04-27T07:05:54Z

I think that once the tests pass, the PR is ready to review.

Ok, the tests passed. PR ready to review.

1. We will assume that we will work with manifolds in which are cells are oriented as the global manifold. 2. For the Piola Map, cell_orientation is not actually necessary for RT FEs on Manifolds if want uses the approach in #579

fverdugo

@amartinhuertas thanks for the PR!

I have some concerns about the introduced coupling between DiscreteModel and Raviart-Thomas reffes. See below.

fverdugo · 2021-04-27T14:34:22Z

src/FESpaces/ConformingFESpaces.jl

  ctype_reffe, cell_ctype = compress_cell_data(cell_reffe)
  ctype_num_dofs = map(num_dofs,ctype_reffe)
  ctype_ldof_comp = map(reffe->get_dof_to_comp(reffe),ctype_reffe)
  cell_conformity = CellConformity(cell_reffe)
-  cell_shapefuns = lazy_map(get_shapefuns,cell_reffe,cell_map)
-  cell_dof_basis = lazy_map(get_dof_basis,cell_reffe,cell_map)
+  cell_shapefuns = get_shapefuns(model,cell_reffe)


Do you need to pass a full discrete model here? is not enough to do something like

cell_shapefuns = lazy_map(get_shapefuns,cell_reffe,cell_map, cell_flip)

?

Do you need to pass a full discrete model here?

We shall agree that there is some work to do at the global FE space level to compute the (cell-wise restriction) of global shape functions and global DoFs in the case of global H(div)-conforming FE spaces (that, e.g., is not required to be done for global grad-conforming FE spaces). In the case of your code snippet, it would be to build the cell_flip array out of the model. In the case of the code I have written, those lines of code which I added to ConformingFESpaces.jl. The code which is in charge of doing this work cannot go to the ReferenceFEs module, otherwise we would have a cycle. I guess that you are trying to avoid this cycle by having interfaces that do not depend on model, and that thus can go to ReferenceFEs.jl. But I am not sure if this is a good solution, because of reasons which I try to make clear below.

I understand the concern of having code in ConformingFESpaces.jl which depends on the particular kind of global FE space at hand. In order to solve this, I propose to add a new source file, DivConformingFEspaces.jl, in FESpaces. See
https://github.com/gridap/Gridap.jl/pull/579/files#diff-ea5ed427dd3dd1fad920c1f2014e8950d5866705bde29f7278c6d93390ae72c1R1 for a motivation. In a nutshell, this source file implements those customizations of get_shape_funs and get_dof_basis for the global conforming FE space at hand.

is not enough to do something like

cell_shapefuns = lazy_map(get_shapefuns,cell_reffe,cell_map, cell_flip)

?

I am not sure if this would be convenient.

First, you would be passing the cell_flip array in order to build the (cell-wise restriction) of global shape functions for all global conforming FE spaces, even if it is not needed (e.g., for Grad-Confirming spaces).

Second, it is hard to anticipate if cell_flip is the info which will be needed in order to compute the (cell-wise restriction) of global shape functions for those global conforming FE spaces which we may want to implement in the future.

By having a get_shapefuns interface with model, and the ReferenceFE, we have more flexibility in defining the global shape functions according to the global FE space at hand. If the name is confusing, we can perhaps think of another name, instead of reusing the one that is used in order to compute the reference FE shape functions.

Let me know what do you think.

fverdugo · 2021-04-27T14:42:19Z

src/FESpaces/ConformingFESpaces.jl

+           get_cell_map(Triangulation(model)))
+end
+
+function get_dof_basis(model::DiscreteModel,


This includes a dependency between discrete model and RaviartThomas reffes. As a result, RaviartThomas reffes cannot be fully implemented in the ReferenceFEs module, which is sort of confusing. It is also a sign that the current API of ReferenceFEs is not general enough.

Do you think it's possible to fix this? Perhaps it is possible with some refactoring of the ReferenceFEs API.

See #579 (comment)

I have been talking to @amartinhuertas this morning, and I think this is the way to go. Div-conforming FE spaces require to modify the reference basis when transformed to the local FE basis. That is the identity for Lagrangian FEM (grad-conforming) but not the case (in general) for other FEs like div or curl-conforming ones.

This is something that cannot be at the ref FE level (it is a global thing, to enforce div-conformity in the global space).

I also don't like to have dispatching wrt RT in the ConformingFESpace file. We have also learned that the only modification that is required to go to div-conforming are these two functions that compute the global DOF and shape functions on physical cells. So, it seems to me that the most neat solution is to create a file for these FE spaces that simply implement these functions.

It is in the same spirit as e.g. discontinuous Galerkin and conforming FE spaces, which fill call the unconstrained FE constructor in different ways. We could also do the dispatching at that level for div-conforming, but since the code is the same with only the previous exception, I would just implement the functions that need dispatching.

Cheers,

--Santi

We can deal with this If it is necessary and I would also rename the versions taking the discrete model to get_cell_shapefuns and get_cell_dof_basis like other functions returning cell-wise values.

Back in the day, get_shapefuns and get_dof_basis had only a single argument (the reference FE), but we included a second argument (the geometrical map) to handle RT and Nedelec. Now these FE types will be handled differently, so perhaps we can revert to the original definition of get_shapefuns and get_dof_basis with just a single argument. This would simplify the implementation of Lagrangian reffes, which became a bit dirty after introducing RT and Nedelec.

In summary, these would the API

# Abstract methods in ReferenceFEs module get_shapefuns(reffe::ReferenceFE) get_dof_basis(reffe::ReferenceFE) # Methods with a default implementation in terms of previous ones (which will work for e.g. Lagrangian), # but they can be overloaded by new ReferenceFE types get_cell_shapefuns(model::DiscreteModel,cell_reffe::AbstractArray{<:ReferenceFE})) get_cell_dof_basis(model::DiscreteModel,cell_reffe::AbstractArray{<:ReferenceFE})) # Methods to be added for RT FEs # 1) Helper methods specific for RT _transformed_rt_shapefuns(reffe::GenericRefFE{RaviartThomas},map::Field,signflip::AbstractVector{<:Integer}) _transformed_rt_dof_basis(reffe::GenericRefFE{RaviartThomas},map::Field,signflip::AbstractVector{<:Integer}) # 2) Specialization of Abstract methods in ReferenceFEs module get_shapefuns(reffe::GenericRefFE) # Already implemented get_dof_basis(reffe::GenericRefFE) # Already implemented # 3) Specialization of cell-wise (like already implemented in the PR) get_cell_shapefuns(model::DiscreteModel,cell_reffe::AbstractArray{<:GenericRefFE{RaviartThomas}})) get_cell_dof_basis(model::DiscreteModel,cell_reffe::AbstractArray{<:GenericRefFE{RaviartThomas}}))

What do you think?

Btw, the sign is not +1 or -1, not an Int but an array with as many +1/-1 as DOFs in the RefFE

Ok! I have updated the code snipped

I used Bool, not Int. (true-> -1 false -> 1; not a big deal)

Done in 1685e4d

Div-conforming FE spaces to a new source file

fverdugo · 2021-04-28T07:01:14Z

src/FESpaces/DivConformingFESpaces.jl

+
+function get_sign_flip(model::DiscreteModel,
+                       cell_reffe::AbstractArray{<:GenericRefFE{RaviartThomas}})
+    lazy_map(get_sign_flip,


(Minor comment). Perhaps, it is easier to pre compute the the cell-wise sign flip into a standard julia vector of Int8 instead to try to make it lazy. It would be also significantly faster if you need to perform several loops over the vector, e.g. in non-linear problems

The array is not as simple, it is an array of arrays of Int. So, I would not proceed this way.

Then better lazy!

I would slightly refactor the definition of the kernel from which the lazy array is defined to match other similar kernels in Gridap. Here, the cell operation depends on a global state (the model). In this case, we usualy include the global state in a struct:

struct SignFlipMap{T} <: Map model::T end function return_cache(k::SignFlipMap,reffe,cell) model = k.model # same code as below end function evaluate!(cache,k::SignFlipMap,reffe,cell) # same code as below end # usage lazy_map(SignFlipMap(model),cell_reffe,cell_ids)

Done in 1685e4d

fverdugo · 2021-04-28T07:41:49Z

src/FESpaces/DivConformingFESpaces.jl

+    sign_flip = cache
+
+    setsize!(sign_flip, (num_dofs(reffe),))
+    sign_flip .= false


Replace this:

sign_flip = cache setsize!(sign_flip, (num_dofs(reffe),))

by

setsize!(cache, (num_dofs(reffe),)) sign_flip = cache.array

Then sign_flip (and the return of this function) will be a intrinsic julia array. This is also a common pattern used in Gridap to hide the cached vector from the user of this function.

Done in 1685e4d

…g_rt_fes_for_arbitrarily_oriented_meshes

spaces \# Abstract methods in ReferenceFEs module get_shapefuns(reffe::ReferenceFE) get_dof_basis(reffe::ReferenceFE) \# Methods with a default implementation in terms of previous ones (which will work for e.g. Lagrangian), \# but they can be overloaded by new ReferenceFE types get_cell_shapefuns(model::DiscreteModel,cell_reffe::AbstractArray{<:ReferenceFE})) get_cell_dof_basis(model::DiscreteModel,cell_reffe::AbstractArray{<:ReferenceFE})) \# Methods to be added for global RT FE spaces \# 1) Helper methods specific for RT _transformed_rt_shapefuns(reffe::GenericRefFE{RaviartThomas},map::Field,signflip::AbstractVector{<:Integer}) _transformed_rt_dof_basis(reffe::GenericRefFE{RaviartThomas},map::Field,signflip::AbstractVector{<:Integer}) \# 2) Specialization of cell-wise (like already implemented in the PR) get_cell_shapefuns(model::DiscreteModel,cell_reffe::AbstractArray{<:GenericRefFE{RaviartThomas}})) get_cell_dof_basis(model::DiscreteModel,cell_reffe::AbstractArray{<:GenericRefFE{RaviartThomas}}))

fverdugo · 2021-04-28T15:13:23Z

@amartinhuertas the renaming get_cell_dof_bais -> get_fe_basis is not correct.

It should be something like

get_cell_shapefuns(::FESpace) -> get_fe_basis
get_cell_shapefuns_trial(::FESpace) -> get_trial_fe_basis
get_cell_dof_basis(::FESpace) -> get_fe_dof_basis

There are the current functions in FESpace that are called get_cell_x and do not return an array

fverdugo · 2021-04-28T15:19:37Z

After the renaming, you can introduce the new functions get_cell_shapefuns(model,cell_reffes) and get_cell_dof_basis(model,cell_reffes).

So the final API would be:

get_fe_basis(fe_space) -> FEBasis
get_trial_fe_basis(fe_space) -> FEBasis
get_fe_dof_basis(fe_space) -> CellDof
get_cell_shapefuns(model,cell_reffes) -> AbstractArray{<:AbstractVector{<:Field}}
get_cell_dof_basis(model,cell_reffes) -> AbstractArray{<:AbstractVector{<:Dof}}

…79/checks?check_run_id=2463353534#step:6:159 __collect_cell_space was calling the old interface of get_shapefuns

amartinhuertas · 2021-04-29T06:14:58Z

@amartinhuertas the renaming get_cell_dof_bais -> get_fe_basis is not correct.

Ooops! You are right (too much rushing). Now it should be solved.

Note also the following change that I had to do in order to pass the tests:

45d5f60

I am not sure if it still makes sense to have trian as a parameter in the interface of compute_cell_space. What do you think? should we remove this parameter ?

fverdugo · 2021-05-04T05:53:25Z

Hi @amartinhuertas!

The purpose of CellFE and CellConformity is to store the raw data from which a general conforming space is constructed. These are potentially more general than a cell wise vector of reference fes plus a Comformity object (i.e., the user could define a strange space that is continuous only at certain facets in a way that cannot be defined in terms of reference fes). Thus, any metadata like a Conformity object should not be included in CellConformity.

Why do you need to add Comformity inside CellConformity?

amartinhuertas · 2021-05-04T06:06:46Z

The purpose of CellFE and CellConformity is to store the raw data from which a general conforming space is constructed. These are potentially more general than a cell wise vector of reference fes plus a Comformity object (i.e., the user could define a strange space that is continuous only at certain facets in a way that cannot be defined in terms of reference fes). Thus, any metadata like a Conformity object should not be included in CellConformity.

Ok, thanks for the clarification. I think now I understand what you say. Thus, as I suspected (but didn't see), you were foreseeing uses cases that I am preventing with the last changes that I introduced (and should thus rollback).

Why do you need to add Comformity inside CellConformity?

Point 1. i in #579 (comment). See also #579 (comment)

fverdugo · 2021-05-04T06:13:33Z

How are we going to implement this line if conformity is of type CellConformity ?

I would provide 2 constructors:

one taking CellFE and CellConformity (the low level one)
another taking a vector of ReferenceFEs and Conformity

(in the 2 cases the second object can be optional since a default value can be taken from the first one)

If the user wants to construct a FE space from a vector of ReferenceFEs plus a CellConformity, one can always build a CellFE from the reference fes and use the 1st constructor.

fverdugo · 2021-05-04T06:19:29Z

If the user wants to construct a FE space from a vector of ReferenceFEs plus a CellConformity, one can always build a CellFE from the reference fes and use the 1st constructor.

One can even add these code into a 3rd constructor if necessary.

amartinhuertas · 2021-05-04T06:57:46Z

I would provide 2 constructors:

Ok, I see. I will come back to you soon with (hopefully) the final version.

a5f3d7f. Finally, we have two constructors of FESpace FESpace(model,cell_reffes,conformity=nothing) FESpace(model,cell_fe,cell_conformity::CellConformity) On the other hand, CellFE constructor becomes CellFE(model,cell_reffes,conformity=nothing)

amartinhuertas · 2021-05-04T10:36:40Z

Ok, I see. I will come back to you soon with (hopefully) the final version.

Ok, done. Remarks:

CellConformity no longer has a member variable of type Conformity.
We have now a CellFE(model,cell_reffes,conformity=nothing) function (as we sketched at the beginning, except for the fact that conformity is though to be either nothing or a valid symbol, instead of an instance of type Conformity).
"The FESpace constructor does not have a cell_conformity parameter (as discussed above). I did not see why this is necessary if CellFE already has cell_conformity as a member variable. (perhaps I am missing a use case when you foresaw this)." This no longer applies. The FESpace constructor now has an optional cell_conformity argument.
"I added an optional keyword argument conformity to the FiniteElements constructor." This still applies.
"Several methods of CellConformity are no longer necessary (currently commented out just in case we have to roll back some of these changes)." This still applies. Not sure why. Can we remove the methods of CellConformity which we are not calling?
"In PhysicalFEsTests.jl, as a consequence of the changes introduced, I was forced to create a different CellFE instance for L2 and nothing (i.e., H1) conformity. Before the changes in this PR, the same CellFE instance was re-used for both calls to FESpace. I guess that know this makes sense because you may have different CellFE instances depending on the global notion of conformity of the FE space." This still applies.

fverdugo · 2021-05-04T13:01:33Z

except for the fact that conformity is though to be either nothing or a valid symbol

The input should be a Conformity object, which is the extensible way of doing dispatch. Why you need a symbol?

amartinhuertas · 2021-05-05T00:03:11Z

The input should be a Conformity object, which is the extensible way of doing dispatch. Why you need a symbol?

It is not actually needed. I can (and I will) definitely use a Conformity instance as a third parameter of the CellFE constructor. I did not do it because of two reasons:

I waived the construction of the Conformity object to the caller, but construct it inside of the CellFE constructor instead.
Dispatching on Conformity is already done inside the CellFE constructor, at the get_cell_dof_basis and get_cell_shapefuns level, and I did not foresee a use case in which the gained generality by using a Conformity parameter at the CellFE constructor would actually be used.

In any case, I agree that using a Conformity parameter opens the door for (unforeseen) possibilities in the future, so that I will follow your advice.

its third parameter

amartinhuertas · 2021-05-05T00:53:54Z

I can (and I will) definitely use a Conformity instance as a third parameter of the CellFE constructor.

Done in 5e1afaf.

Please continue your code review taking #579 (comment) as a basis. You will see there some questions that require an answer.

santiagobadia · 2021-05-05T05:17:16Z

src/FESpaces/FESpaceFactories.jl

-  cell_fe::CellFE;
-  conformity=nothing,
+  cell_fe::CellFE,
+  cell_conformity::CellConformity=CellConformity(cell_fe);


I would not include a positional argument here. CellConformity is in CellFE and I don't think we want/consider the case in which we have different CellConformity in CellFE than the one passed in this constructor.

ok, agreed. I do not see either why this is necessary, and can be even counterproductive.

[skip ci]

amartinhuertas · 2021-05-05T08:38:01Z

Ok, done. I think the PR is more or less ready to be merged up to following comment that still needs to be addressed:

"Several methods of CellConformity are no longer necessary (currently commented out just in case we have to roll back some of these changes)." This still applies. Not sure why. Can we remove the methods of CellConformity which we are not calling?

what do u think @fverdugo?

amartinhuertas · 2021-05-05T08:45:07Z

I am in particular referring to these three methods:

function CellConformity(cell_fe::CellFE,cell_conf::Nothing)
  cell_fe.cell_conformity
end

function CellConformity(cell_fe::CellFE,cell_conf::CellConformity)
  @assert length(cell_fe.cell_ctype) == length(cell_conf.cell_ctype)
  cell_conf
end

# function CellConformity(cell_fe::CellFE,conformity::Union{Symbol,Conformity})
#   if conformity in (L2Conformity(),:L2)
#     @notimplemented
#   elseif conformity == :default
#     cell_fe.cell_conformity
#   else
#     @unreachable """\n
#     Argument conformity should be either :default, :L2, or L2Conformity()
#     """
#   end
# end

fverdugo · 2021-05-05T11:40:55Z

I am in particular referring to these three methods:

I agree that they are not longer needed. They can be removed.

fverdugo

Hi @amartinhuertas !

Some more comments!

I found that the name get_cell_shapefuns was already correct in the Geometry module (the new name get_fe_basis does not make sense in this context).

See inlined-comments.

fverdugo · 2021-05-05T11:48:42Z

src/FESpaces/DivConformingFESpaces.jl

+#   shape function corresponding to the global DoF.
+# * We do NOT have to use the signed determinant, but its absolute value, in the Piola Map.
+
+function get_cell_dof_basis(model::DiscreteModel,


It looks that when using RT FEs with L2 conformity, the default cell shape funs will be generated (i.e. the ones without the transformation). Is this enough?

Yes this is enough because no conformity is to be imposed

fverdugo · 2021-05-05T11:52:32Z

src/Geometry/AppendedTriangulations.jl

@@ -70,9 +70,9 @@ end
 #  lazy_append(a,b)
 #end

-function get_cell_shapefuns(trian::AppendedTriangulation)


Here (in the Geometry mdoule) the name get_cell_shapefuns was already correct, since the result is the array of cell-wise shape functions.

Revert the renaming in this module and add import Gridap.Geometry: get_cell_shapefuns in FESpaces.jl

ok, good call. Thanks for detecting this out!

fverdugo · 2021-05-05T11:53:58Z

test/CellDataTests/CellDofsTests.jl

@@ -17,7 +17,7 @@ model = simplexify(CartesianDiscreteModel(domain,cells))

 trian = Triangulation(model)

-v = GenericCellField(get_cell_shapefuns(trian),trian,ReferenceDomain())


Idem as before, here the old name was correct!

get_fe_basis in the Geometry module

amartinhuertas · 2021-05-06T00:26:44Z

Some more comments!

Done, ready to be accepted! (once the tests are passing, I will post a comment when that happens)

refactoring

665ce26

amartinhuertas · 2021-05-06T02:01:01Z

Done, ready to be accepted! (once the tests are passing, I will post a comment when that happens)

All tests passing! Thanks @fverdugo and @santiagobadia for your support! I think we can now accept the PR.

Some changes towards a generalized implementation of RT FEs

766d472

that does not assume an oriented global mesh. Not passing tests. To investigate.

amartinhuertas requested a review from santiagobadia April 14, 2021 10:38

amartinhuertas changed the title ~~Some changes towards a generalized implementation of RT FEs that does not assume an oriented global mesh.~~ Towards a generalized implementation of RT FEs that does not assume an oriented global mesh. Apr 14, 2021

[RaviartThomasFEs] Implemented sign flip in slave cells when

20b6c6c

global DoFs are transformed into local DoFs

amartinhuertas added 2 commits April 26, 2021 16:56

Merge branch 'master' of github.com:gridap/Gridap.jl into generalizin…

ccf8568

…g_rt_fes_for_arbitrarily_oriented_meshes

amartinhuertas requested a review from fverdugo April 27, 2021 03:45

amartinhuertas mentioned this pull request Apr 27, 2021

Misc changes in order to support RT FEs in general manifolds (i.e., num_cell_dims(model) < num_point_dims(model)) #577

Merged

fverdugo requested changes Apr 27, 2021

View reviewed changes

Moved the code in ConfirmingFESpaces.jl that is particular to

9d4b337

Div-conforming FE spaces to a new source file

fverdugo reviewed Apr 28, 2021

View reviewed changes

amartinhuertas added 2 commits April 28, 2021 23:15

Merge branch 'master' of github.com:gridap/Gridap.jl into generalizin…

42cfbe8

…g_rt_fes_for_arbitrarily_oriented_meshes

amartinhuertas added 5 commits April 29, 2021 13:20

Undo renaming get_cell_dof_basis -> get_fe_basis

5d40170

get_cell_shapefuns_trial(::FESpace) -> get_trial_fe_basis

10932df

get_cell_shapefuns(::FESpace) -> get_fe_basis

7bc9744

get_cell_dof_basis(::FESpace) -> get_fe_dof_basis

78b3798

Solved not passing test in https://github.com/gridap/Gridap.jl/pull/5…

45d5f60

…79/checks?check_run_id=2463353534#step:6:159 __collect_cell_space was calling the old interface of get_shapefuns

Modified CellFE constructor so that it gets a Conformity instances as

5e1afaf

its third parameter

santiagobadia requested changes May 5, 2021

View reviewed changes

amartinhuertas added 2 commits May 5, 2021 15:37

Removed CellConformity as a positional argument of FESpace constructor

85da28f

Minor changes to documentation

78a2a43

[skip ci]

fverdugo requested changes May 5, 2021

View reviewed changes

Fixed accidental renaming of get_cell_shapefuns by

665ce26

get_fe_basis in the Geometry module

amartinhuertas added 4 commits May 6, 2021 10:28

Pending renaming from 665ce26

a7ad74d

More pending renamings from Pending renaming from 665ce26

b0adfe4

Removed CellConformity methods which are no longer required after

56f2d1b

refactoring

Fixing two renaming mistakes from

571462f

665ce26

fverdugo self-requested a review May 6, 2021 07:15

fverdugo approved these changes May 6, 2021

View reviewed changes

fverdugo merged commit 84fcf88 into master May 6, 2021

fverdugo deleted the generalizing_rt_fes_for_arbitrarily_oriented_meshes branch May 6, 2021 07:50

amartinhuertas mentioned this pull request May 12, 2021

RT FEs on manifolds (writing in progress) #571

Closed

		@@ -17,7 +17,7 @@ model = simplexify(CartesianDiscreteModel(domain,cells))

		trian = Triangulation(model)

		v = GenericCellField(get_cell_shapefuns(trian),trian,ReferenceDomain())

Towards a generalized implementation of RT FEs that does not assume an oriented global mesh. #579

Towards a generalized implementation of RT FEs that does not assume an oriented global mesh. #579

Conversation

amartinhuertas commented Apr 14, 2021 • edited Loading

santiagobadia commented Apr 14, 2021

amartinhuertas commented Apr 23, 2021 • edited Loading

amartinhuertas commented Apr 23, 2021 • edited Loading

amartinhuertas commented Apr 27, 2021

amartinhuertas commented Apr 27, 2021

codecov-commenter commented Apr 27, 2021 • edited Loading

Codecov Report

amartinhuertas commented Apr 27, 2021

fverdugo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fverdugo Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fverdugo commented Apr 28, 2021

fverdugo commented Apr 28, 2021

amartinhuertas commented Apr 29, 2021

fverdugo commented May 4, 2021 • edited Loading

amartinhuertas commented May 4, 2021

fverdugo commented May 4, 2021

fverdugo commented May 4, 2021

amartinhuertas commented May 4, 2021

amartinhuertas commented May 4, 2021

fverdugo commented May 4, 2021

amartinhuertas commented May 5, 2021

amartinhuertas commented May 5, 2021

Choose a reason for hiding this comment

amartinhuertas May 5, 2021 • edited Loading

Choose a reason for hiding this comment

amartinhuertas commented May 5, 2021

amartinhuertas commented May 5, 2021 • edited Loading

fverdugo commented May 5, 2021

fverdugo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amartinhuertas commented May 6, 2021

amartinhuertas commented May 6, 2021

amartinhuertas commented Apr 14, 2021 •

edited

Loading

amartinhuertas commented Apr 23, 2021 •

edited

Loading

amartinhuertas commented Apr 23, 2021 •

edited

Loading

codecov-commenter commented Apr 27, 2021 •

edited

Loading

fverdugo Apr 28, 2021 •

edited

Loading

fverdugo commented May 4, 2021 •

edited

Loading

amartinhuertas May 5, 2021 •

edited

Loading

amartinhuertas commented May 5, 2021 •

edited

Loading