Generalization of FESpaceWithLastDofRemoved #396

amartinhuertas · 2020-09-05T02:06:15Z

As agreed, this PR:

Replaces FESpaceWithLastDofRemoved by FESpaceWithDofPotentiallyRemoved, a more general version with two additional features:
1. One can select the dof to remove.
2. One can activate or deactivate dof removal using a True/False trait (I know that you do not like True/False traits, but it in this case there are certainly only two options).
Modifies ZeroMeanFESpace such that it now relies on FESpaceWithDofPotentiallyRemoved.

(not directly related to this PR, but FYI, FESpaceWithDofPotentiallyRemoved is already being succesfully used by the parallel distributed-memory version of ZeroMeanFESpace; click here for more details).

To simplify things, I think that two main concerns should be object to code review:

A better name for FESpaceWithDofPotentiallyRemoved? (I used the one that we came up as a joke, but after thinking a bit, I have no better alternative).
As the dof to remove may not be the first or the last dof, I had to introduce extra memory allocations wrt FESpaceWithLastDofRemoved. In particular, here and here. Can we leave with that? If not, do you foresee any possible solution to avoid this?

@amartinhuertas

…fPotentiallyRemoved. FESpaceWithDofPotentiallyRemoved is though to be a future replacement for FESpaceWithLastDofRemoved, as it is based on the same concept, but it is more general as it lets the user to activate/deactivate whether to remove the dof at hand, and select the particular dof to be removed

…hLastDofRemoved, but on FESpaceWithDofPotentiallyRemoved,

…th_dof_potentially_removed

fixed as an input argument (instead of being hard-coded to the last dof)

santiagobadia · 2020-09-07T03:26:00Z

src/FESpaces/FESpacesWithDofPotentiallyRemoved.jl

@@ -0,0 +1,185 @@
+"""
+    struct FESpaceWithDofPotentiallyRemoved{CS,RD} <: SingleFieldFESpace


FESpaceConstantFixed or FESpaceWithConstantFixed

Note the difference between having the constants fixed and zero mean value

I will use FESpaceWithConstantFixed. The "potential" sense of the type is lost in the name, but we can leave with that.

Note the difference between having the constants fixed and zero mean value

Ok, good point. This space lets you fix the constant to an arbitrary value. The zero mean space fixes it to a particular value, i..e, the one which results in a zero mean function.

This space lets you fix the constant to an arbitrary value.

FYI, the zero mean FE space currently assumes that the value is fixed exactly to 0 in the fixed dof of the FESpaceWithDofPotentiallyRemoved.

FYI, the zero mean FE space currently assumes that the value is fixed exactly to 0 in the fixed dof of the FESpaceWithDofPotentiallyRemoved.

I do not see this statement. Doesn't it depend on the dirichlet values that come along with the trial FE space?

Done in faa32a3

santiagobadia · 2020-09-07T03:27:14Z

src/FESpaces/FESpacesWithDofPotentiallyRemoved.jl

+  function FESpaceWithDofPotentiallyRemoved(space::SingleFieldFESpace,
+     remove_dof::Bool,
+     dof_to_remove::Int=num_free_dofs(space))
+    s = "FESpaceWithDofPotentiallyRemoved can only be constructed from spaces without dirichlet dofs."


Probably do nothing in this case. It reminds me Balancing Neumann-Neumann local spaces. If the constant is already fixed, do nothing. Minor point.

Done in 0da971d.

santiagobadia · 2020-09-07T03:32:57Z

src/FESpaces/FESpacesWithDofPotentiallyRemoved.jl

+  dv .= view(_fv,f.dof_to_remove:f.dof_to_remove)
+  (fv, dv)
+end
+


In all these functions, we should decide which is the way to deal with the vector with one row eliminated in an efficient way. We could probably create a lazy vector doing this, using what we already have in Gridap. It is a quite obvious and would not involve any allocation instead of using views + vcat. Idem for the vector with only one touched value.

Another option is to implement a VectorWithIndexRemoved in addition to the VectorWithIndexInserted I commented above.

I will implement VectorWithIndexRemoved and VectorWithIndexInserted because it will be even more efficient than using lazy vectors, and can be useful for other scenarios as well. Besides, lazy vectors do not currently support write operations, broadcasted assignments, etc. Is there any fundamental reason behind this? or it is just because it is missing? I do not need it, anyway.

Done in PR #401

santiagobadia · 2020-09-07T03:48:58Z

src/FESpaces/FESpacesWithDofPotentiallyRemoved.jl

+    r[j] = rj
+  end
+  r
+end


Do we need this? Couldn't we just change the data vector in the Table that represents the face dofs using a lazy vector that only touches one entry (to -1)? Probably create a method for adding these constraints in face_own_dofs Table.

I think we need this (or something in this direction) to be general. We cannot assume in general that we have a Table for the face dofs. We could write (on top of this) a specialized version when we have a Table, but it seems premature optimization to me.

It does not have to be a table but just create a method that modifies the face_own_dofs whatever it is. The idea is simple, only modify the plain global vector by touching one of, in the line of what you say above, e.g., VectorWithIndexChanged (horrible name) with the global Ids that is being used in face_own_dofs. This way, we do not need the code above, I guess.

I worked on all the issues, except this one.

Just to double-check, you propose to remove the CellDofsWithDofFixed data type, right?

Plus a possible alternative solution, that I do not enterily grasp. What I do not get is:

"Probably create a method for adding these constraints in face_own_dofs Table."

"It does not have to be a table but just create a method that modifies the face_own_dofs whatever it is."

face_own_dofs is a Table returned by ReferenceFE right? How is the link among this Table and get_cell_dofs() such that we can achieve the desired effect?. I am sure that they key is here, and thus I am missing something important.

I have been exploring get_cell_dofs references along the whole project, and there are a myriad of different data types that such method can return. Of course, all this data types can be conceptually seen as Vector{Vector{Int}}. Besides, such methods, can be traversed in several ways, using a single loop, two nested loops, with and without caches, etc.

What it comes into my mind is to define a new Vector of vectors, which is built from another vector of vectors, and two indices, i, j, and a value v, such that the new vector fulfills x[i][j]==v. But this is in some sense what CellDofsWithDofFixed is doing, so I expect to be missing something.

santiagobadia · 2020-09-07T03:54:25Z

src/FESpaces/FESpacesWithDofPotentiallyRemoved.jl

+  space::SingleFieldFESpace
+  constraint_style::Val{CS}
+  remove_dof::Val{RD}
+  dof_to_remove::Int


I would use structs for these values, not true or false. Then, you don't need constraint_style and remove_dof, already in the param. You can create obvious getter for these. More readable code and more extensible.

Proposal (feedback welcomed):

abstract type ConstantStrategy end; struct FixConstant <: ConstantStrategy end struct DoNotFixConstant <: ConstantStrategy end

With constraint_style I need help. May be @fverdugo can help here? What does True/False mean for the constraint_style of FE spaces? On the other hand, I guess this is a cross-cutting change, right?

With constraint_style I need help. May be @fverdugo can help here? What does True/False mean for the constraint_style of FE spaces? On the other hand, I guess this is a cross-cutting change, right?

constraint_style(V) == Val(false) for un-constrained spaces

Done (partially) in 0da971d. In particular, I would prefer to transform constraint_style into struct-like traits in a different PR. I expect that such implies a number of changes that deserve its separate PR.

fverdugo · 2020-09-07T06:26:57Z

src/FESpaces/FESpacesWithDofPotentiallyRemoved.jl

+  _dv = similar(dv,eltype(dv),0)
+  _fv = vcat(view(fv,1:f.dof_to_remove-1),
+             dv,
+             view(fv,f.dof_to_remove:length(fv))) # TODO lazy append


Try Gridap.Arrays.lazy_append

The other option is to implement a VectorWithIndexInserted, or whatever name you like.

fverdugo · 2020-09-07T06:28:41Z

src/FESpaces/FESpacesWithDofPotentiallyRemoved.jl

+    r[j] = rj
+  end
+  r
+end


I think we need this (or something in this direction) to be general. We cannot assume in general that we have a Table for the face dofs. We could write (on top of this) a specialized version when we have a Table, but it seems premature optimization to me.

fverdugo · 2020-09-07T06:30:39Z

src/FESpaces/FESpacesWithDofPotentiallyRemoved.jl

+
+function gather_free_and_dirichlet_values!(
+  fv,dv,f::FESpaceWithDofPotentiallyRemoved{CS,true},cv) where {CS}
+  _fv, _dv = gather_free_and_dirichlet_values(f.space,cv)


Can we reduce code duplication between gather_free_and_dirichlet_values and gather_free_and_dirichlet_values! ?

Can we reduce code duplication between gather_free_and_dirichlet_values and gather_free_and_dirichlet_values! ?

I guess the comment has become outdated after latest changes. Agreed? (See below)

function gather_free_and_dirichlet_values( f::FESpaceWithConstantFixed{CS,FixConstant},cv) where {CS} _fv, _dv = gather_free_and_dirichlet_values(f.space,cv) @assert length(_dv) == 0 fv = VectorWithEntryRemoved(_fv,f.dof_to_fix) dv = _fv[f.dof_to_fix:f.dof_to_fix] (fv, dv) end function gather_free_and_dirichlet_values!( fv,dv,f::FESpaceWithConstantFixed{CS,FixConstant},cv) where {CS} _fv, _dv = gather_free_and_dirichlet_values(f.space,cv) @assert length(_dv) == 0 fv .= VectorWithEntryRemoved(_fv,f.dof_to_fix) dv[1] = _fv[f.dof_to_fix] (fv, dv) end

fverdugo · 2020-09-07T06:41:12Z

src/FESpaces/FESpacesWithDofPotentiallyRemoved.jl

+  dv .= view(_fv,f.dof_to_remove:f.dof_to_remove)
+  (fv, dv)
+end
+


Another option is to implement a VectorWithIndexRemoved in addition to the VectorWithIndexInserted I commented above.

fverdugo · 2020-09-07T06:46:24Z

src/FESpaces/FESpacesWithDofPotentiallyRemoved.jl

@@ -0,0 +1,185 @@
+"""
+    struct FESpaceWithDofPotentiallyRemoved{CS,RD} <: SingleFieldFESpace


This space lets you fix the constant to an arbitrary value.

FYI, the zero mean FE space currently assumes that the value is fixed exactly to 0 in the fixed dof of the FESpaceWithDofPotentiallyRemoved.

…ed.jl

constan in FESpaceWithConstantFixed

…ith_dof_potentially_removed

…Integer,value)

codecov-commenter · 2020-09-10T16:57:28Z

Codecov Report

Merging #396 into master will decrease coverage by 0.23%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##           master     #396      +/-   ##
==========================================
- Coverage   87.49%   87.26%   -0.24%     
==========================================
  Files         156      158       +2     
  Lines       11049    11092      +43     
==========================================
+ Hits         9667     9679      +12     
- Misses       1382     1413      +31

Impacted Files	Coverage Δ
src/Arrays/Arrays.jl	`100.00% <ø> (ø)`
src/FESpaces/FESpaces.jl	`100.00% <ø> (ø)`
src/Arrays/VectorsWithEntryInserted.jl	`50.00% <50.00%> (ø)`
src/Arrays/VectorsWithEntryRemoved.jl	`50.00% <50.00%> (ø)`
src/FESpaces/FESpacesWithConstantFixed.jl	`75.34% <75.34%> (ø)`
src/FESpaces/ZeroMeanFESpaces.jl	`89.09% <88.88%> (-1.48%)`	⬇️

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 ce0e1f1...df737b8. Read the comment docs.

fverdugo · 2020-09-11T06:27:41Z

@amartinhuertas just a minor thing. Can you add a temporarily alias

export FESpaceWithLastDofRemoved
const FESpaceWithLastDofRemoved = FESpaceWithConstantFixed

and comment in NEWS.md, deprecated section, that " the name FESpaceWithLastDofRemoved has been deprecated in favor of FESpaceWithConstantFixed"

With this, the code is 100% backwards compatible and we can publish your changes via v0.14.1 ASAP so that you can use a registered Gridap version in GridapDistributed.

We need to remember to delete the alias in 0.15.

amartinhuertas · 2020-09-11T06:37:14Z

Sure! But take into account that it is not a single renaming. The interface of the constructor has changed. Do we still follow the deprecated path?

fverdugo · 2020-09-11T06:49:44Z

Sure! But take into account that it is not a single renaming. The interface of the constructor has changed. Do we still follow the deprecated path?

Then, use @deprecate as much as you need like in

Gridap.jl/src/FESpaces/SingleFieldFESpaces.jl

Line 172 in 5faa6e0

@deprecate(

amartinhuertas · 2020-09-11T07:06:30Z

I was not precise enough in my previous message, sorry about that.

The new data type has introduced a lot of changes with respect to the previous data type. It has a a new template parameter! I can start using deprecate everywhere, but I have the impression that I am some how maintaining the previous data type alive. I am not sure it this is what we want. I guess that you want to avoid a new minor relese right? (0.15)

fverdugo · 2020-09-12T06:24:25Z

the old type is a trivial particular case of the new one. We only need these temporary defs until 0.15:

export FESpaceWithLastDofRemoved
const FESpaceWithLastDofRemoved{CS} = FESpaceWithConstantFixed{CS,FixConstant}
@deprecate FESpaceWithLastDofRemoved(space::SingleFieldFESpace) = FESpaceWithConstantFixed(space,true)

this 3 lines allow us to publish 0.14.1 ASAP so that you can use it in GridapDistributed

fverdugo · 2020-09-12T06:24:59Z

@amartinhuertas ☝️

fverdugo · 2020-09-12T06:26:37Z

FYI, from the julia doc:

As of Julia 1.5, functions defined by @deprecate do not print warning when julia is run without the --depwarn=yes flag set, as the default value of --depwarn option is no. The warnings are printed from tests run by Pkg.test().

amartinhuertas · 2020-09-12T06:57:01Z

the old type is a trivial particular case of the new one. We only need these temporary defs until 0.15:

Ok, good call. I was expecting something more dirty to keep the old data type alive. New lesson learned! Done!

amartinhuertas · 2020-09-12T07:17:48Z

I realized that this PR was already merged, perhaps accidentally? I will open a brand new one, I cannot re-open this one.

…oved Fe space with dof potentially removed (pending changes from PR #396)

amartinhuertas added 7 commits August 25, 2020 19:41

Modified ZeroMeanFESpace such that it no longer depends on FESpaceWit…

a5e00a1

…hLastDofRemoved, but on FESpaceWithDofPotentiallyRemoved,

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

e020506

…th_dof_potentially_removed

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

e67c140

…th_dof_potentially_removed

Generalized _compute_new_fixedval such that it takes the dof which is

4a9fa27

fixed as an input argument (instead of being hard-coded to the last dof)

Removed FESpaceWithLastDofRemoved

638fe89

Removed export from FESpaces.jl

b358318

amartinhuertas requested review from santiagobadia and fverdugo September 5, 2020 02:06

santiagobadia reviewed Sep 7, 2020

View reviewed changes

fverdugo reviewed Sep 7, 2020

View reviewed changes

amartinhuertas added 3 commits September 10, 2020 12:10

Renamed FESpaceWithDofPotentiallyRemoved.jl by FESpaceWithConstantFix…

faa32a3

…ed.jl

Using Traits in order to control whether to fix or do not fix the

0da971d

constan in FESpaceWithConstantFixed

Added VectorWithEntryInserted and VectorWithEntryRemoved

01c1fa4

amartinhuertas mentioned this pull request Sep 10, 2020

Added VectorWithEntryInserted and VectorWithEntryRemoved #401

Merged

amartinhuertas and others added 7 commits September 11, 2020 00:28

Update NEWS.md

e4c7b87

Update NEWS.md

9ea5851

Solved syntax error in FESpaceWithConstantFixed

d7f3bc6

Merge branch 'vector_with_entry_inserted_and_removed' into fe_space_w…

4412327

…ith_dof_potentially_removed

Missing test files

7cad067

Bug-fix in function VectorWithEntryInserted(a::AbstractVector,index::…

7ac6e1e

…Integer,value)

Adapted FESpacesWithConstantFixed to new vectors developed

df737b8

fverdugo approved these changes Sep 11, 2020

View reviewed changes

fverdugo merged commit fbea57c into master Sep 11, 2020

fverdugo deleted the fe_space_with_dof_potentially_removed branch September 11, 2020 06:20

amartinhuertas mentioned this pull request Sep 12, 2020

Fe space with dof potentially removed (pending changes from PR #396) #404

Merged

fverdugo added a commit that referenced this pull request Sep 12, 2020

Merge pull request #404 from gridap/fe_space_with_dof_potentially_rem…

c1e0e6a

…oved Fe space with dof potentially removed (pending changes from PR #396)

amartinhuertas mentioned this pull request Sep 14, 2020

Potential Improvements in current implementation of FESpaceWithConstantFixed #406

Closed

		@@ -0,0 +1,185 @@
		"""
		struct FESpaceWithDofPotentiallyRemoved{CS,RD} <: SingleFieldFESpace

Generalization of FESpaceWithLastDofRemoved #396

Generalization of FESpaceWithLastDofRemoved #396

Conversation

amartinhuertas commented Sep 5, 2020

Choose a reason for hiding this comment

amartinhuertas Sep 7, 2020 • 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

santiagobadia Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amartinhuertas Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

santiagobadia Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

amartinhuertas Sep 7, 2020 • 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

codecov-commenter commented Sep 10, 2020

Codecov Report

fverdugo commented Sep 11, 2020

amartinhuertas commented Sep 11, 2020

fverdugo commented Sep 11, 2020

amartinhuertas commented Sep 11, 2020

fverdugo commented Sep 12, 2020

fverdugo commented Sep 12, 2020

fverdugo commented Sep 12, 2020

amartinhuertas commented Sep 12, 2020

amartinhuertas commented Sep 12, 2020

amartinhuertas Sep 7, 2020 •

edited

Loading

santiagobadia Sep 7, 2020 •

edited

Loading

amartinhuertas Sep 11, 2020 •

edited

Loading

santiagobadia Sep 7, 2020 •

edited

Loading

amartinhuertas Sep 7, 2020 •

edited

Loading