Transient distributed cell field

[oriolcg]

Hi @santiagobadia, @amartinhuertas, @fverdugo,

This is a draft PR that migrates the developments done in GridapODEs to support GridapDistributed into GridapDistributed. This is something that we discussed with @santiagobadia in the Slack channel. Please, take a look at the modifications and let me know what do you think.

The current status already works for the transient Poisson (heat equation) and Stokes equation, provided that GridapODEs is linked to the branch support_GridapDistributed

[fverdugo]

Have you considered to add this work to GridapODEs and include GridapDistributed in the dependencies of GridapODEs?

This is the patter we have followed with the other packages. Eg. GridapGmsh and GridapPETSc depend on GridapDistributed, not viceversa.

[oriolcg]

Have you considered to add this work to GridapODEs and include GridapDistributed in the dependencies of GridapODEs?

Yes, this is what I had already working in 3718467c399bda379329ee2d99bbeafec8a240dd. This migration came after a discussion with @santiagobadia (see the "#gridap-odes" channel in Slack).

[fverdugo]

I think it is significantly better that the extension packages depend on GridapDistributed, as we are doing now.

Otherwise, GridapDistributed would grow indefinitely large since it would potentially incorporate parallel code of all extension packages. As a result, the size of GridapDistribtued would be proportional to the number of extension packages.

Moreover, I think it is also conceptually better to have everything related with gmsh in GridapGmsh (idepedentendly if it is serial or parallel code, this is just a detail), everything related with petsc in GridapPETSc, everything related with ODEs in GridapODEs etc.

[santiagobadia]

@fverdugo

Gridap is a serial package and GridapDistributed is the layer that provides distributed-memory parallelism.The FE part is in Gridap and the parallel FE part is in GridapDistributed now. If this is the pattern for the spatial discretisation, I don't see why not for the time discretisation. For me, this is the consistent way to go.

GridapP4est, GridapGmsh, GridapPETSc are different packages, which are already parallelism-aware. In fact, they provide functionalities to GridapDistributed, not the other way around.

A user that wants to run in serial (most common case) is going to use Gridap / GridapODEs and no dependency with GridapDistributed is required. If this user now wants to make the driver parallel, they are going to add GridapDistributed. I think this is a clean and consistent way to go. Only when one wants to go to parallel, MPI, OpenMP, P4est, PETSc [...] will be downloaded.

But far more important, we don't want to add parallel tests to GridapODEs. It is much better to have the parallel tests in GridapDistributed. We know that including parallel tests is challenging, we have already suffered this in GridapDistributed. It makes no sense to suffer this in such a light package as GridapODEs (sysimage, precompilation, etc).

GridapDistributed is already a very light package, 2500 LOC. Adding a bunch of lines for GridapODEs is not a problem at all.

So, I propose to go the way used in this PR.

A deeper discussion that we can postpone (even though somehow related) is to create vs not to create new discretisation packages (add the functionality to Gridap in a new submodule). It is quite subjective. E.g., why DG methods are in Gridap and hybridised methods are in GridapHybrid? We probably want to start thinking about merging some curated packages. I don't have a strong opinion here. But using the approach suggested, it would be easier in the future, if needed. There are pros and cons.

[amartinhuertas]

In my view, it is reasonable to put the parallelization of GridapODEs in GridapDistributed, as in this PR. (putting it in GridapODEs is reasonable as well, but I believe that it adds some burdens to the user/developer of GridapODEs that leans the scale on the approach in this PR).

If GridapODEs was a part of Gridap (this would also be reasonable, as @santiagobadia mentions), there would not be discussion, right?. Thus, I also believe this PR is the consistent way to go with the Gridap-GridapDistributed relation. As a bonus, you dont complicate the workflow, github actions, etc. of a package that was originally though to be a serial package.

On the other hand, we have reduced GridapDistributed to the minimum. It is essentially a wrapper that combines different building blocks. While we are adding more LOCs of code to support GridapODEs parallelization (concern of @fverdugo), the code that we are adding it is in nature equivalent to the one that we already have in GridapDistributed for spatial discretization (wrapper types, combining building blocks, etc.) , thus this seems to be consistent as well.

Finally, note that from the perspective of a user that wants to implement a parallel transient solver, there is essentially no difference among one decision or the other.

[amartinhuertas]

Something that we can always do is to put the parallelization code of GridapODEs into its own module of GridapDistributed, instead of in the main trunk. Just an idea, still thinking ....

[amartinhuertas]

Something that we can always do is to put the parallelization code of GridapODEs into its own module of GridapDistributed, instead of in the main trunk. Just an idea, still thinking ....

I mean to have a submodule within GridapDistributed, called GridapODEs, and put all the code in this PR in GridapDistributed.GridapODEs. Note we do not have yet module hierarchy in GridapDistributed.

[santiagobadia]

Sure! I think it makes sense. And the same for tests. This way it is easy to cut.

@oriolcg you could create this submodule in the same PR.

Btw, the tests are not passing yet.

[oriolcg]

Ok, I'll create a submodule for this.

Btw, the tests are not passing yet.

The test are not passing because the CI does not link with the branch where I have the corresponding changes in GridapODEs. After merging and registering a new version they should work.

[fverdugo]

GridapODEs was a part of Gridap there would not be discussion, right?

But it's not. To be consistent with this, I believe that all things that are related with GridapODEs should be in GridapODEs. Otherwise, we are breaking the modularity of the ecosystem in GridapDistributed.

As pointed @santiagobadia, the actual discussion is which packages should be in Gridap and which ones are extension packages. But if some functionality happens to not be merged in Gridap, please do not merge it in GridapDistributed.

[santiagobadia]

As I said, I have asked @oriolcg to put this dev here for consistency and simplicity. I think that we do not share the consistency view. The other fact (parallel tests in GridapODEs) is also an important reason to keep the dev here. Let us be practical. Especially when the so-called consistency view is not shared by the others.

I don't see how we are breaking any modularity. This is subjective.

I think creating a Submodule here is the best way to go.

[oriolcg]

GridapODEs submodule created in d1bfa5e. The tests should be passing once gridap/GridapODEs.jl#68 is released.

[fverdugo]

I am not confortable adding GridapODEs to the dependencies here since this will cause all present and future packages with parallel support to depend on GridapODEs indirectly (objective fact). Eg GridapGmsh, GridapPETCs, GridapP4Est, GridapMHD, etc. in most of them there is no reason to depend on GridapODEs. GridapODEs is not a small package and it also has its own indirect and direct dependencies.

For me, the extra effort to create parallel tests in GridapODEs (we have done this in the other parallel packages) pays off avoiding unnecessary dependencies in all other unrelated packages with parallel support.

[oriolcg]

Hi @santiagobadia, @amartinhuertas, @fverdugo. How should we proceed with that? I would like to unlock this development soon. Would it work to have a dedicated meeting to go over pros-cons or possible alternatives?

[santiagobadia]

Since we are thinking about merging GridapODEs with Gridap, the parallel ODE part will be here.
In the meantime, I think it is OK to accept these changes and we can just eliminate the GridapODEs dependency when merged (minor modification).

[santiagobadia]

@oriolcg

I have merged GridapODEs into Gridap. Can you update to 0.17.9?

After this, please, eliminate the GridapODEs dependency in Project.toml and modify using GridapODEs... by using Gridap.GridapODEs....

Then, we can accept the PR

[oriolcg]

Done in d98f459. The tests should pass after releasing PR gridap/Gridap.jl#751 and PR fverdugo/PartitionedArrays.jl#74.

[codecov-commenter]

Codecov Report

Merging #81 (5166991) into master (4036e66) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master     #81    +/-   ##
=======================================
  Coverage    0.00%   0.00%            
=======================================
  Files           7      10     +3     
  Lines        1338    1453   +115     
=======================================
- Misses       1338    1453   +115     

Impacted Files	Coverage Δ
src/TransientDistributedCellField.jl	0.00% <0.00%> (ø)
src/TransientFESpaces.jl	0.00% <0.00%> (ø)
src/TransientMultiFieldDistributedCellField.jl	0.00% <0.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[oriolcg]

The tests are passing with the last Gridap release.

@fverdugo suggested to not add a submodule for ODEs, but probably this can be done in another PR.

[oriolcg]

Hi @amartinhuertas @fverdugo @santiagobadia, can this PR be accepted?

[fverdugo]

As a minor remark, I would remove the module ODEs to follow the same pattern as in the other parts of the code. E.g., there is no module MultiField in GridapDistributed. For the rest, OK.

[oriolcg]

As a minor remark, I would remove the module ODEs to follow the same pattern as in the other parts of the code. E.g., there is no module MultiField in GridapDistributed. For the rest, OK.

Done in 5166991