Higher-order implicit Runge-Kutta methods for the partitioned heat equation by NiklasVin · Pull Request #415 · precice/tutorials

NiklasVin · 2023-11-29T16:17:52Z

With this implementation, you could compute the partitioned heat equation with any implicit RK method, which is provided
by ButcherTableaux.py file.
You just need to change the time stepping scheme tsm to the IRK you want to use.
The weak form is obtained algorithmically and does not need to be changed by the user.

Previously, there were cases, where an empty dictionary was returned by the function. Additionally, runtime errors which occured when using single step methods are now fixed, as sub() was not defined for the Function space which is used for ssm

…oned-heat-conduction/fenics

…e time derivative!

… t=dt

With the change, the function shouldn't get into trouble if it created a BSpline with a degree >3

Refactoring: Avoid special treatment for single stage

WIP: Import from FEniCS and simplify code.

The time of the i-th stage was computed incomprehensible. This was changed by just setting the time to t+c[i]*dt

BenjaminRodenberg

Couldn't resist taking a look at the PR 😏 Generally looks good. I did not check the actual results, but this is something you are doing in much more detail than I can do it here. Therefore, I was mainly focusing on code style.

We should also try to directly move some minor changes to develop, but this is something I can do.

BenjaminRodenberg · 2023-11-29T18:57:21Z

partitioned-heat-conduction/fenics/heat.py

 import dolfin
-from dolfin import FacetNormal, dot
+from dolfin import FacetNormal, dot, project


Why do we use dolfin here? Let's stick to from fenics import .... I think FacetNormal is not needed, is it?

project is provided by dolfin. And as the imports were already there, I used them.
Actually, also regarding the current implementation of heat.py, all imports from dolfin can be deleted, because dot is already imported above from fencis, and, as you said, FacetNormal is not used.

But you can also use from fenics import project. I would suggest: Let's remove all dolfin imports here. We can then cherry-pick this change to develop, because you are of course right that it is not 100% the point of this PR to clean up the imports and therefore we should also not mix this up.

partitioned-heat-conduction/fenics/heat.py

partitioned-heat-conduction/fenics/utils/utils.py

partitioned-heat-conduction/fenics/heat.py

BenjaminRodenberg · 2023-12-01T19:44:39Z

I just cleaned up the unnecessary dolfin imports via 71f8daa. Please merge develop into this PR to make the diff a bit more compact.

…e B-Spline interpolation

BenjaminRodenberg

Minor readability comments. If this means that you have to start with all your testing from the beginning: You can also ignore these suggestions and we just apply this refactoring after this PR is merged.

BenjaminRodenberg · 2023-12-11T21:51:01Z

partitioned-heat-conduction/fenics/utils/utils.py

+    for i in range(num_stages):
+        uhelp = initial_condition
+        for j in range(num_stages):
+            uhelp = uhelp + tsm.A[i][j] * dt * ks[j]
+        u[i] = uhelp


Suggested change

for i in range(num_stages):

uhelp = initial_condition

for j in range(num_stages):

uhelp = uhelp + tsm.A[i][j] * dt * ks[j]

u[i] = uhelp

for i in range(num_stages):

u[i] = initial_condition

for j in range(num_stages):

u[i] += tsm.A[i][j] * dt * ks[j]

BenjaminRodenberg · 2023-12-11T22:08:34Z

partitioned-heat-conduction/fenics/utils/utils.py

+    rh = 0
+    for i in range(num_stages):
+        rh = rh + f[i] * vs[i] * dx
+
+    # Assemble weak form from lhs and rhs
+    F = 0
+    for i in range(num_stages):
+        # cf. https://doi.org/10.1145/3466168 p.5 equation 14
+        F = F + weak_lhs(v=vs[i], u=u[i], k=ks[i])
+    F = F - rh
+    return F


Suggested change

rh = 0

for i in range(num_stages):

rh = rh + f[i] * vs[i] * dx

# Assemble weak form from lhs and rhs

F = 0

for i in range(num_stages):

# cf. https://doi.org/10.1145/3466168 p.5 equation 14

F = F + weak_lhs(v=vs[i], u=u[i], k=ks[i])

F = F - rh

return F

# Assemble weak form

F = 0

for i in range(num_stages):

# cf. https://doi.org/10.1145/3466168 p.5 equation 14

F += inner(ks[i], vs[i]) * dx + inner(grad(u[i]), grad(vs[i])) * dx - f[i] * vs[i] * dx

return F

I don't really see a good reason for introduction weak_lhs here. Currently, the whole script does not generalize that well, if you want to solve a different equation anyway, because the f comes from "the outside". But this is also ok here.

If you want to get rid of the index, you can also try:

for u, v, k, f in zip(us, vs, ks, fs): F += inner(k, v) * dx + inner(grad(u), grad(v)) * dx - f * v * dx

btw: Renaming the u from above to us and the f to fs I think would not harm. Currently, it's a bit odd that you say vs, but not us.

BenjaminRodenberg · 2024-01-05T09:08:53Z

I think we should definitely try to get this merged into the tutorials, because it is an important test for waveforms in preCICE. But I'm still a bit unsure how we should continue here: For the sake of the review it was very useful to modify heat.py, but to me it is not clear whether we should still keep the simple version of the tutorial (without stages, just implicit Euler directly solving the system for u).

So there are the following three options:

Replace heat.py with the advanced high-order version. Drop the old and simple implicit Euler version.
Provide high-order as heat.py, but also keep a heatImplicitEuler.py in this folder.
Leave heat.py as it is, but provide heatHigherOrder.py in this folder.

Then, there is, of course, also the option to move this case into an entirely new folder tutorials/partitioned-heat-conduction-high-order, but I think this would not give this contribution the right level of attention.

Opinions @uekerman and @MakisH?

MakisH · 2024-01-05T10:00:21Z

partitioned-heat-conduction/fenics/heat.py

+The implementation of the higher-order implicit Runge-Kutta methods was mostly taken from:
+    https://github.com/NikoWul/FenicsIrksome


FYI, the original repository has no license. I understand that @BenjaminRodenberg was related to (supervised?) that project, but it would be best to make this clear.

MakisH · 2024-01-05T10:11:45Z

I think we should definitely try to get this merged into the tutorials, because it is an important test for waveforms in preCICE.

Definitely! No good work should see slow decay in unmerged PRs.

But I'm still a bit unsure how we should continue here: For the sake of the review it was very useful to modify heat.py, but to me it is not clear whether we should still keep the simple version of the tutorial (without stages, just implicit Euler directly solving the system for u).

My main argument here would be consistency: We have invested a lot of effort in having all solver options of each tutorial behave in similar ways and give similar results. By merging this as-is, it means that FEniCS will be doing something different than the rest of the solver options.

So there are the following three options:

* Replace `heat.py` with the advanced high-order version. Drop the old and simple implicit Euler version.

That would be the breaking case, so for me, this is a clear "no".

* Provide high-order as `heat.py`, but also keep a `heatImplicitEuler.py` in this folder.

This changes the default behavior, and hence the consistency with the rest, so also "no" for me.

* Leave `heat.py` as it is, but provide `heatHigherOrder.py` in this folder.

This sounds like the easiest, least intrusive option. How easy would it be to merge the two and make the implicit solver a special case, configurable at runtime? That could prevent some duplication.

Then, there is, of course, also the option to move this case into an entirely new folder tutorials/partitioned-heat-conduction-high-order, but I think this would not give this contribution the right level of attention.

We typically do this when we need a different precice-config.xml, which is not the case here. It would mainly lead to duplication.

My recommendation: Leave heat.py as-is, but provide the higher order solver as a special case (either in the same code or, if that is complicated, in a separate file).

partitioned-heat-conduction/fenics/heat.py

…tation to a new file and reset heat.py to state of the dev branch

BenjaminRodenberg

Looks good to me 👍 Will merge this now and apply some final changes directly on develop.

NiklasVin and others added 30 commits October 20, 2023 20:08

Initial commit for higher order time stepping schemes

129fc9c

Bug in derivative approximation fixed

d050ca2

Previously, there were cases, where an empty dictionary was returned by the function. Additionally, runtime errors which occured when using single step methods are now fixed, as sub() was not defined for the Function space which is used for ssm

Bug fixes

2b13810

Bspline interpolation

89c750b

Implemented B-Spline time derivative approximation

95c6966

Move files of partitioned-heat-conduction/fenics_highOrder to partiti…

2469325

…oned-heat-conduction/fenics

Fixed issue with time derivative. Only Dirichlet participant needs th…

c9f7e67

…e time derivative!

Make B-Spline method more resilient againt rounding errors at t=0 and…

4cffd72

… t=dt

Enable different waveform degree

850bb11

Setup with discontinuous solutions at the coupling boundary

00007d7

Print more digits

51c3720

Analytic coupling boundary test

8bc4c1d

Wann erhält man einen kleinen Fehler?

e182ae6

Fehler in einem Test behoben

fe5d6bd

Import from FEniCS and simplify code.

b1f1639

Create individual coupling expressions for each stage.

fba2d24

Add note.

140afa4

Cleanup.

fef8f25

Use time at end of time step for Neumann BC

29fa60b

Initialize heat flux properly.

d410f66

Avoid special treatment for single stage

075b651

Update partitioned-heat-conduction/fenics/heat.py

1b9d9cb

Update utils.py

b149bcd

With the change, the function shouldn't get into trouble if it created a BSpline with a degree >3

Merge pull request #2 from BenjaminRodenberg/refactoring_single_stage

6f4310c

Refactoring: Avoid special treatment for single stage

Merge branch 'bspline_interpolation' into review_2023_11_16

a19d9a3

Merge pull request #1 from BenjaminRodenberg/review_2023_11_16

51d5d9c

WIP: Import from FEniCS and simplify code.

Tidy up code

4c4d9d1

Changed odd time derivative computation

b87cd4e

The time of the i-th stage was computed incomprehensible. This was changed by just setting the time to t+c[i]*dt

Added missing source

bc2903e

Merge

85da623

NiklasVin added 6 commits November 25, 2023 17:15

Bad merge fix

453b480

Comments

ba37e9f

Added missing reference of ButcherTableaux.py

a3a80c8

Format

b3d10a4

Format

2ef3d38

Added references to github repositories

902511e

BenjaminRodenberg suggested changes Nov 29, 2023

View reviewed changes

Suggested changes

eaa57f4

NiklasVin mentioned this pull request Nov 30, 2023

API for getting the derivative of the waveform precice/precice#1908

Open

NiklasVin added 2 commits December 4, 2023 12:27

Merge and format

ff0fef6

Changed some comments and the number of samples which are used for th…

d6219da

…e B-Spline interpolation

BenjaminRodenberg reviewed Dec 11, 2023

View reviewed changes

MakisH reviewed Jan 5, 2024

View reviewed changes

BenjaminRodenberg reviewed Jan 5, 2024

View reviewed changes

partitioned-heat-conduction/fenics/heat.py Outdated Show resolved Hide resolved

Suggested changes applied in utils.py, move the higher order implemen…

bbc6e48

…tation to a new file and reset heat.py to state of the dev branch

BenjaminRodenberg approved these changes Jan 18, 2024

View reviewed changes

BenjaminRodenberg merged commit 222da83 into precice:develop Jan 18, 2024

BenjaminRodenberg mentioned this pull request Feb 12, 2024

Implement higher (2nd) order time-stepping scheme in fenics example precice/fenics-adapter#30

Closed

NiklasVin deleted the bspline_interpolation branch June 17, 2024 18:13

BenjaminRodenberg mentioned this pull request Aug 30, 2024

Add pointers to higher-order time-stepping schemes #560

Closed

3 tasks

		The implementation of the higher-order implicit Runge-Kutta methods was mostly taken from:
		https://github.com/NikoWul/FenicsIrksome

Uh oh!

Conversation

NiklasVin commented Nov 29, 2023

Uh oh!

BenjaminRodenberg left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminRodenberg Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

NiklasVin Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

BenjaminRodenberg Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BenjaminRodenberg commented Dec 1, 2023

Uh oh!

BenjaminRodenberg left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminRodenberg Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

BenjaminRodenberg Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

BenjaminRodenberg commented Jan 5, 2024

Uh oh!

MakisH Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

MakisH commented Jan 5, 2024

Uh oh!

Uh oh!

BenjaminRodenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants