Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split partitioned-heat-conduction case into complex and simple one #148

Merged
merged 28 commits into from
Mar 5, 2021

Conversation

BenjaminRodenberg
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg commented Jan 20, 2021

closes #147

Specification for partitioned-heat-conduction:

  • Dirichlet is always left, Neumann is always right
  • Coupling Boundary is always straight line parallel to y axis
  • No time dependent heat flux and rhs
  • mpirun is still supported (for fenics)
  • no subcycling. Timestep of both FEniCS solvers is hard-coded to be equal to time window size from precice-config.xml.
  • Analytical solution is always possible
  • use scalar-valued flux (important for compatibility with nutils and again easier)

Specification for partitioned-heat-conduction-complex:

  • Dirichlet and Neumann solvers are arbitrary
  • Coupling Boundary is arbitrary
  • Time dependent heat flux and rhs may be set via gamma
  • mpirun is supported (for fenics)
  • subcycling possible
  • Analytical solution is possible for straight and circular boundary as long as no subcycling is used.
  • use vector-valued flux

Open Todos

@BenjaminRodenberg
Copy link
Member Author

@uekerman I assume that flow-over-heated-plate is independent from this PR? The FEniCS solver used in flow-over-heated-plate is very similar, but has some differences and cannot easily be reused.

@uekerman
Copy link
Member

@uekerman I assume that flow-over-heated-plate is independent from this PR? The FEniCS solver used in flow-over-heated-plate is very similar, but has some differences and cannot easily be reused.

yes, flow-over-heated-plate is completely independent of this one.

@uekerman
Copy link
Member

Specification looks good so far. I guess a real review only makes sense once we have the compatibility with Nutils.
A comment already now: for that we will need a scalar flux and a nearest-neighbor mapping.

@BenjaminRodenberg
Copy link
Member Author

Specification looks good so far. I guess a real review only makes sense once we have the compatibility with Nutils.
A comment already now: for that we will need a scalar flux and a nearest-neighbor mapping.

To make sure I understand: We want this case for FEniCS and for nutils. Meaning, according to the new structure we get:

  • partitioned-heat-conduction/
    • solid-fenics/...
      • heat.py
    • solid-nutils/...
      • heat.py
    • precice-config.xml
    • ...

This means, that we need a new tutorial (according to the old structure), precice/tutorials/tree/master/HT/partitioned-heat/nutils-nutils, first. Does this already exist somewhere? The closest one I can find is flow-over-plate/buoyantPimpleFoam-nutils.

I think the cleanest approach would be to first create another PR from develop, where I collect all the necessary parts from partitioned-heat/fenics-fenics and flow-over-plate/buoyantPimpleFoam-nutils to develop a new case partitioned-heat/nutils-nutils. Afterwards I can use the resulting case, add it in this PR and move everything according to the new structure.

@uekerman
Copy link
Member

uekerman commented Feb 3, 2021

This means, that we need a new tutorial (according to the old structure), precice/tutorials/tree/master/HT/partitioned-heat/nutils-nutils, first. Does this already exist somewhere?

Yes. Sources are provided in the issue: #137

To make sure I understand: We want this case for FEniCS and for nutils. Meaning, according to the new structure we get:

The in #137 suggested structure is

- dirichlet-fenics
  - heat.py
- neumann-fenics
  - heat.py
- dirichlet-nutils
  - heat.py
- neumann-nutils
  - heat.py 
- precice-config.xml
- clean.sh
- README.md

Yes, the drawback of this scheme is that it leads to quite some code duplication. Maybe it is simple to understand though.

Alternatively:

- fenics
  - heat.py
- nutils
  - heat.py
- precice-config.xml 

together with some command line argument to decide whether the left Dirichlet or the right Neumann sub domain should be computed.

@BenjaminRodenberg BenjaminRodenberg changed the title Simplify partitioned-heat-conduction case Split partitioned-heat-conduction case into complex and simple one Feb 3, 2021
@BenjaminRodenberg
Copy link
Member Author

This means, that we need a new tutorial (according to the old structure), precice/tutorials/tree/master/HT/partitioned-heat/nutils-nutils, first. Does this already exist somewhere?

Yes. Sources are provided in the issue: #137

I added the sources to this PR. Currently just copy-paste without any testing or updates of the code.

To make sure I understand: We want this case for FEniCS and for nutils. Meaning, according to the new structure we get:

The in #137 suggested structure is

- dirichlet-fenics
  - heat.py
- neumann-fenics
  - heat.py
- dirichlet-nutils
  - heat.py
- neumann-nutils
  - heat.py 
- precice-config.xml
- clean.sh
- README.md

Yes, the drawback of this scheme is that it leads to quite some code duplication. Maybe it is simple to understand though.

That's probably a good reason not to use this structure here.

Alternatively:

- fenics
  - heat.py
- nutils
  - heat.py
- precice-config.xml 

together with some command line argument to decide whether the left Dirichlet or the right Neumann sub domain should be computed.

I like this idea. Added to this PR 👍

@BenjaminRodenberg
Copy link
Member Author

I took care of updating the nutils case as far as I can get. I currently see one problem:

Nutils uses two meshes and FEniCS only one. This does not allow us to use the same configuration for both cases. @uekerman any idea what we can do here?

@uekerman
Copy link
Member

uekerman commented Feb 3, 2021

Nutils uses two meshes and FEniCS only one. This does not allow us to use the same configuration for both cases. @uekerman any idea what we can do here?

We could read and write at the GPs only, that should be simple

@BenjaminRodenberg
Copy link
Member Author

The last problem with this PR that I see is that the exact solution cannot be recovered when using nutils. Remaining features are fine.

@BenjaminRodenberg BenjaminRodenberg marked this pull request as ready for review February 5, 2021 15:08
@BenjaminRodenberg
Copy link
Member Author

BenjaminRodenberg commented Feb 5, 2021

I decided to postpone fixing the nutils case to #152 and #153. This PR is from my point of view ready to merge review. We can continue working on #153 at a later point in time from my point of view and I don't want with merging the features that are already working until the nutils case is fixed, since I'm a bit stuck here currently and it is hard to estimate how long fixing this will take.

@BenjaminRodenberg
Copy link
Member Author

Notes for review:

  • Are the specifications from above fulfilled?
  • Are there still any features or files we don't need for the respective cases?
  • Is the documentation clear (enough)?

Copy link
Member

@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 The separation of simple and complex geometry cases makes the FEniCS code much more readable :)

@BenjaminRodenberg
Copy link
Member Author

@uekerman another hint for the review: Currently we don't provide fenics/run.sh, since we need command line input to decide whether we run the Dirichlet or Neumann solver. I see two options here:

  1. provide a fenics/run.sh that accepts command line input
  2. provide a dirichlet-fenics/run.sh and neumann-fenics/run.sh. Put all actual code only into dirichlet-fenics. neumann-fenics only exists for following the general structure and providing a run.sh script.

Any opinion?

Copy link
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall structure looks good. Minor conventions things and README stuff below.

{% include important.html content="We have not yet ported the documentation of the preCICE tutorials from the preCICE wiki to here. Please go to the [preCICE wiki](https://github.com/precice/precice/wiki#2-getting-started---tutorials)" %}
## Setup

In this tutorial we solve a partitioned heat equation. For information on the non-partitioned case, please refer to [1, p.37ff]. The corresponding FEniCS code can be found on [github:fenics-tutorial](https://github.com/hplgit/fenics-tutorial/blob/master/pub/python/vol1/ft03_heat.py).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs probably a bit more information. We have not yet mentioned FEniCS before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I provided more details on the case in 75e68af. I don't think that we have to explain what FEniCS is here. Or what do you mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, "FEniCS" falls a bit from the sky. The setup should actually be independent of the solver.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some reordering in 9614c02. I think this makes it a bit better.

@uekerman
Copy link
Member

Currently we don't provide fenics/run.sh, since we need command line input to decide whether we run the Dirichlet or Neumann solver. I see two options here:

1. provide a `fenics/run.sh` that accepts command line input

2. provide a `dirichlet-fenics/run.sh` and `neumann-fenics/run.sh`. Put all actual code only into `dirichlet-fenics`. `neumann-fenics` only exists for following the general structure and providing a `run.sh` script.

Any opinion?

I guess we are also fine without a run script here for the moment. If we add some later then option 1.

Copy link
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Did not run it again, but did not see any obvious problems.

@uekerman uekerman merged commit caa5ad0 into restructure Mar 5, 2021
@BenjaminRodenberg BenjaminRodenberg deleted the i_137 branch September 28, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add complex geometry fenics-fenics tutorial
3 participants