First prototype of the dune-precice module (dune-adapter) for preCICE#1
First prototype of the dune-precice module (dune-adapter) for preCICE#1davidscn merged 13 commits intoprecice:mainfrom
Conversation
|
Thanks for the contribution, @maxfirmbach 🎉 |
|
@maxfirmbach thank you very much for the contribution! Before reviewing it, do you maybe have a case that we can run it with? If you want, you can also open a separate PR in the tutorials. |
|
@MakisH I can create a tutorial case and will do a PR in the tutorials (might however take some time). To run a test case the DUNE core modules and some extension modules are needed and the first installation of the whole DUNE environment can be a bit cumbersome. |
|
Regarding the installation of DUNE. There is a Spack recipe for DUNE. Alternatively, there is an installation script for DuMuX. The installation script also installs DUNE dependencies. The dependency list in line 134 needs to be adapted for the DUNE adapter to contain |
|
I added a case, where Dune acts as solid-participant in the perpendicular-flap tutorial case. The corresponding folder inside the tutorial directory should also be ready. |
davidscn
left a comment
There was a problem hiding this comment.
I tried to install the module but there was a problem (see below)
The directory structure is actually fine (dune-precice).
| https://github.com/maxfirmbach/dune-elastodynamics | ||
|
|
||
| The adapter can be build with: | ||
| `<path-to-dune-common/bin/dunecontrol> --current all` |
There was a problem hiding this comment.
I installed dune from apt, where dunecontrol is in the global searchpath:
| `<path-to-dune-common/bin/dunecontrol> --current all` | |
| `dunecontrol --current all` |
There was a problem hiding this comment.
Maybe link here also to the module installation instructions provided by DUNE itself. Is the dunecontrol make install command missing?
There was a problem hiding this comment.
I guess that are the instructions on the website: https://www.dune-project.org/doc/installation/
|
|
||
| // setup looping | ||
| double t = 0.0; | ||
| double dt = 0.0001; |
There was a problem hiding this comment.
Why do you need to select such a small time-step size? The tutorial uses actually 1e-2?!
There was a problem hiding this comment.
An explicit RKN-method is used in this case and for larger time steps I had difficulties with stability. I guess the tutorial cases use an implicit method.
There was a problem hiding this comment.
Ah ok. This probably explains the deviation of the displacement amplitude as well (I can run a brief comparison tomorrow). You don't have any other time-stepping methods available, right? All in all, I can run the tutorial case andI will have a closer look at the code tomorrow.
There was a problem hiding this comment.
There should also be a family of Newmark methods (might have to clean that up a bit) and adaptive explicit RKN methods. But great that the thing runs at least.
|
I added a implicit time stepping case I also cleaned up the time stepping method in dune-elastodynamics so before trying the new case, get a fresh pull of that please. I did a simulation, which looked fine ... I hope it works for you too. |
Yes, this works as well and it is faster and more compliant to the other tutorials. However, there is still some deviation between our other tutorials and the dune code here, even with the implicit time-stepper (as you probably know) Do you have any ideas about the reasoning? Method-wise, you use continuous finite-elements of polynomial degree 2 and an implicit Newmark time-stepping with |
|
Yes, I use continuous finite-elements of polynomial degree 2 and an implicit Newmark time-stepping with gamma=0.5 and beta=0.25. To be honest, I'm not sure why the result is different ... I already had a close look at all parts and to my best knowledge the structural part is correct. Still I couldn't figure out till now where the deviation comes from ... Maybe a short remark, there's a file called |
|
Ah that's nice, we use so please change this in your elastodynamics module. I will wait for the feedback of @ajaust here for now. |
|
That looks really nice! I can't just change it ... cause for beam bending problems I need plane stress ... but I will find a way to call the right one. Thanks for the help so far! |
ajaust
left a comment
There was a problem hiding this comment.
Thank you again for the adapter and sorry for the late response. I like your work, but nevertheless left some critical remarks. You do not have to fix everything now, but I think there are some things we have to think about and maybe create issues for them to track them. I can also help with some things if needed. I left some remarks in the code.
In short, these are some of the general remarks:
- Make function parameters constant where possible. C++ core guidelines
- Mark functions that do not change any members as const. C++ core guidelines
- Trivial types do not have to be passed by reference. C++ core guidelines
- Could we add some simple test (maybe with a solver dummy)? Ideally with some test that might be run via a GitHub action (I would help with that). We can also move this into an issue for a future version.
- The module name
dune-preciceis good. I would call the repositorydune-preciceinstead ofdune-adapter. This would follow the standard nomenclature used by DUNE projects. It would make sense to change the layout as well by moving everything from the directorydune-precicein the root of the repository. This would be follow structure of other DUNE modules. dune-preciceshould not depend ondune-elastodynamicsin my opinion as the adapter itself does not depend on it. Only the supplied example depends ondune-elastodynamicsanddune-preciceThis means we should do something with the supplied example insrc/.- I did not try to compile or work with the supplied example yet.
- Document the dependency on a preCICE version. Does it work with preCICE v1 or v2? Does it work with both version?
- We should maybe add more documentation and some test (solverdummies)?!
| @@ -1,2 +1,31 @@ | |||
| # dune-adapter | |||
There was a problem hiding this comment.
I think the whole repository should be called dune-precice to follow standard DUNE notation. At the same time the structure of the repository should be adapted accordingly. Namely, all the content in the folder dune-precice should be in the root of the repository.
dune-precice/dune.module
Outdated
| Version: 1.0 | ||
| Maintainer: [email protected] | ||
| #depending on | ||
| Depends: dune-common (>= 2.7) dune-istl (>= 2.7) dune-functions (>= 2.7) dune-elastodynamics |
There was a problem hiding this comment.
Currently, this repository depends on dune-elastodynamics which does not make sense to me. This increases the number of dependencies while I would vote for dune-precice being a baseline implementation that can be used in other DUNE modules with minimum dependencies. Why would I care installing dune-elastodynamics if I do want to use dune-precice for a completely different type of application?
Therefore I would suggest
| Depends: dune-common (>= 2.7) dune-istl (>= 2.7) dune-functions (>= 2.7) dune-elastodynamics | |
| Depends: dune-common (>= 2.7) dune-istl (>= 2.7) dune-functions (>= 2.7) |
and adding a simple example (dummysolver?) that purely relies on dune-precice and its dependencies.
I can see three scenarios at the moment:
- Relaxing the current dependency Make the current example a suggested dependency and prevent automatic building of the example.
| Depends: dune-common (>= 2.7) dune-istl (>= 2.7) dune-functions (>= 2.7) dune-elastodynamics | |
| Depends: dune-common (>= 2.7) dune-istl (>= 2.7) dune-functions (>= 2.7) | |
| Suggests: dune-elastodynamics (>=1.0) |
In this case I would also rename the directory. src/ is not very verbose. Something like examples/elastodynamic/ would be better.
- Move all examples that require further dependencis into its one DUNE module (
dune-precice-examples?). - The current example could live in
dune-elastodynamicsand one could adddune-preciceas a suggested dependency there.
There was a problem hiding this comment.
The dune-precice module is now only depending on the core modules. I moved the examples to dune-precice-howto as this naming convention is also used for tutorial modules of certain core modules.
There was a problem hiding this comment.
Considering dune-precice-examples I was thinking about cutting the dependency to dune-elastodynamics completely in the future and replacing it by dune-pdelab. A linear-elasticity example should be rather easy to construct there.
| for(int i=0; i<dune_to_precice.N(); i++) { | ||
| for(int j=0; j<dim; j++) { | ||
| if(isBoundary[i][j]) { write_data[iteration_count] = dune_to_precice[i][j]; ++iteration_count; } | ||
| } | ||
| } |
There was a problem hiding this comment.
- Could we avoid raw loops here and instead used
std::for_each, for example? See Slide 3 or the actual talk. 😄 - Is
isBoundary[i]not true for all j? I do not know/understand the indexing?! I would expectito be the index of the degree of freedom andjrefer to the coordinates or so.
|
@maxfirmbach do you have any idea which parts you want to address or if you want to address parts at all? Do you need help with some comments? If you don't want to keep working on this we could also merge and add a TODO list. |
|
I can address the changes to the code, but I work quite unregulary on the dune-adapter ... so it always takes some time. Considering the dependencies and the structure of the tutorial examples etc., I guess that's a part, which shouldn't be decided by me. If a merge + ToDo List is ok, I'm fine with that, I will work on the adapter from time to time. |
|
Alright that's totally fine, then let's keep it as it is. I just want to prevent that things become stale. |
|
I restructured the adapter and the tutorial cases. Now there are two DUNE modules inside the |
ajaust
left a comment
There was a problem hiding this comment.
Thank you very much for the work. I went through the main adapter part and did not check the dune-precice. I like the state of the adapter a lot. I have just some minor remarks.
|
|
||
| #include <dune/functions/functionspacebases/interpolate.hh> | ||
|
|
||
| #include <precice/SolverInterface.hpp> |
There was a problem hiding this comment.
Does it make sense to include it inside the Dune::preCICE namespace to keep the scope of preCICE more narrow?
There was a problem hiding this comment.
I'm not sure, does it make a big difference?
There was a problem hiding this comment.
I am not sure what would be the best here. Including it within the namespace would hide preCICE and its precice namespace a bit more from users. From my perspective this would be something good since we want people to use the adapter. However, I did not find any examples regarding good and bad practices. This is for sure a small thing we can also easily change later. I just wanted to mention it.
Maybe @fsimonis has an idea/suggestion regarding the best practices regarding includes inside/outside namespaces?!
There was a problem hiding this comment.
I think we can just keep this way, it is also more clear from an outside view (at least in my opinion), because you have all headers before starting to do anything.
|
I didn't follow the discussions, but I think this PR is ready to merge, @maxfirmbach any concerns? Is the tutorial ready as well? |
|
I agree that it is ready. If there are problems, we can still discuss and change/fix things later. |
|
From my side there are no concerns, at least for now. The tutorial should also be ready. |
|
Thank you for your contribution! |


Hello preCICE-team and developers,
this is a prototype of a module inside the DUNE framework, representing an adapter for the preCICE-library.
For now this module is mainly used for FE calculations, enabling multi-physics simulations within DUNE.
Literature references can be found in the README inside the dune-precice folder.