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

Partitioned heat conduction structure is non-standard and inconsistent #228

Closed
BenjaminRodenberg opened this issue Jul 19, 2021 · 13 comments

Comments

@BenjaminRodenberg
Copy link
Member

BenjaminRodenberg commented Jul 19, 2021

Background

In PR #223 there was already a discussion whether we should change the structure of our partitioned heat conduction tutorial or not (see #223 (comment)). We decided to move this discussion to an independent issue, since it was off-topic for #223.

Current state

The current structure of this tutorial is inconsistent, since the OpenFOAM solver has independent solvers for Dirichlet and Neumann while the FEniCS and Nutils solvers don't:

Rough structure (from #223 (comment)):

├── fenics
├── images
├── nutils
├── openfoam-dirichlet
├── openfoam-neumann
├── openfoam-solver
├── precice-config.xml
└── README.md

Possible solutions

Currently there are three main possibilities:

  1. keep it as it is. 👍
  2. follow with all solvers the standard way, where you have an independent directory for every solver. We will replace fenics and nutils with fenics-dirichlet, fenics-neumann, nutils-dirichlet, nutils-neumann. This would be consistent with all/most of our other tutorials. 👍 👍
  3. try to only have a single OpenFOAM solver. We merge openfoam-dirichlet and openfoam-neumann into openfoam. As far as I understand we concluded in Add partitioned-heat OpenFOAM participant with solver #223 that this is not possible. 👎

Goal of this issue

We should agree in this issue on a structure that fits for cases like the partitioned heat equation, where there are not necessarily two independent solvers, but only a domain decomposition of a single solver happens. This structure should be as consistent with existing tutorials as possible and at the same time not introduce a large amount of code duplication (maintainment!).

@davidscn
Copy link
Member

follow with all solvers the standard way, where you have an independent directory for every solver. We will replace fenics and nutils with fenics-dirichlet, fenics-neumann, nutils-dirichlet, nutils-neumann. This would be consistent with all/most of our other tutorials.

I would vote for this option. This opens the door for alternative contributions as well. Maybe some solver do not support all combinations of data reading and writing.

try to only have a single OpenFOAM solver. We merge openfoam-dirichlet and openfoam-neumann into openfoam. As far as I understand we concluded in Add partitioned-heat OpenFOAM participant with solver #223 that this is not possible.

If we explicitly want to run the case openfoam <-> openfoam, then this is not possible since the mesh is generated separately and stored in default directories. If we exclude the openfoam <-> openfoam case we could make it work using some custom run scripts where the relevant files are copied (i.e., activated) for the current simulation.

@BenjaminRodenberg
Copy link
Member Author

If we exclude the openfoam <-> openfoam case we could make it work using some custom run scripts where the relevant files are copied (i.e., activated) for the current simulation.

This sounds to me like replacing one thing that is non-standard with two other things that are non-standard.

From my point of view option 2 is also the best one (if we want to change something).

@BenjaminRodenberg
Copy link
Member Author

I just started adding 👍 and 👎 above. I understand that there are two 👍 for option 2 from @davidscn and me. There is one 👎 for option 3 from my side. And a 👍 on option 1 from my side - since this is no work. Feel free to edit this list (I think there is no need for a detailed breakdown where the votes come from).

@MakisH
Copy link
Member

MakisH commented Jul 19, 2021

Among these options, I also support option 2 (fenics-dirichlet, fenics-neumann) for consistency and simplicity. I believe this is rather clear, we just didn't want to rename things in the same PR.

Regarding the naming, this is still inconsistent. If Dirichlet and Neumann are also the participant names, then we should name them dirichlet-fenics and neumann-fenics. Assuming that we always have e.g. Dirichlet being the left and Neumann the right.

What is not clear to me is where to store shared files (e.g. solver files). The distinction into openfoam-solver and openfoam-<case> goes already in a good direction, but I would prefer to group all these in a standardized subdirectory and not mix them with the cases. I believe that a <case>/tools/ would be intuitive enough.

@davidscn
Copy link
Member

What is not clear to me is where to store shared files (e.g. solver files). The distinction into openfoam-solver and openfoam- goes already in a good direction, but I would prefer to group all these in a standardized subdirectory and not mix them with the cases. I believe that a /tools/ would be intuitive enough.

For me, tools is something which helps you with certain things such as cleaning or running, but which is not a significant part of the simulation itself. Hence, putting the solver into a tools directory for me counter intuitive and not the right place.

@MakisH
Copy link
Member

MakisH commented Jul 19, 2021

What is not clear to me is where to store shared files (e.g. solver files). The distinction into openfoam-solver and openfoam- goes already in a good direction, but I would prefer to group all these in a standardized subdirectory and not mix them with the cases. I believe that a /tools/ would be intuitive enough.

For me, tools is something which helps you with certain things such as cleaning or running, but which is not a significant part of the simulation itself. Hence, putting the solver into a tools directory for me counter intuitive and not the right place.

Would a directory with a different name, such as solvers, be good enough?

@davidscn
Copy link
Member

solvers is indeed very intuitive. However, I guess that this case here is rather an exception and I am not sure if we really put anything else there than this OpenFOAM solver (also in other cases). Or do we add fenics and nutils there as well? This would probably be inconsistent with other tutorial cases.

@MakisH
Copy link
Member

MakisH commented Jul 19, 2021

As we are trying to extend our current structure to not have special cases, I am trying to imagine more similar cases. Such a case could also be the partitioned-pipe, if we added more cases.

This also originates from the wide range of "adapter" definitions. If solver == adapter and the solver is only tailored a particular scenario (e.g. partitioned HT), we have two cases:

  • (A) can the same adapter/solver act as either partiticipant of this case? --> Should be at the <case> directory level.
  • (B) is the adapter/solver specific to a participant? --> Should be at the <case>/<participant>-<solver> directory level.

We currently have "fused" solvers (A) e.g. in fenics and nutils. Transforming them to (B) would introduce some duplication, but avoid the whole discussion about where to put the solver. We could also transform the openfoam-solver to (B) and then we can store it inside <participant>-<solver> and stick to our general convention, without changing anything.

@uekerman
Copy link
Member

I prefer duplication here over software engineering. Splitting /duplicating the nutils code into two variants is very easy and could also make the code easier to understand. Putting some of the code into a solvers would make things complicated to read. I expect additional lines of code for the software engineering > saved lines code by avoiding the duplication (for nutils).

Regarding the naming, this is still inconsistent. If Dirichlet and Neumann are also the participant names, then we should name them dirichlet-fenics and neumann-fenics. Assuming that we always have e.g. Dirichlet being the left and Neumann the right.

Yes, good point. Your last remark should be indeed a hard definition of this tutorial.

@MakisH
Copy link
Member

MakisH commented Jul 20, 2021

Just to clarify: A solvers or tools directory would only be needed for OpenFOAM. Unless we also want to duplicate that one and distribute it in the <case>/<participant>-openfoam directories.

@BenjaminRodenberg
Copy link
Member Author

This issue is quite old, but to me it reads like we actually concluded on option 2 in the end. Meaning: The action item is to split nutils into nutils-dirichlet and nutils-neumann (same for fenics). Main argument in favor: Simplifies the code because it only needs to deal with either Dirichlet or Neumann in each folder. Please 👍, if this is summarizing the discussion from above or comment, if I'm missing something.

We don't have to start working on this now, but I think it would be good, if we could clearly move from the discussion phase into the doing phase here.

@MakisH
Copy link
Member

MakisH commented Jan 19, 2024

I read again the whole thread. My summary:

  • Rename {openfoam-dirichlet, openfoam-neumann} to {dirichlet-openfoam, neumann-openfoam} and check for similar instances in other tutorials.
  • Split nutils into {dirichlet-nutils, neumann-nutils}
  • Split fenics into {dirichlet-fenics, neumann-fenics}

Even though not crystal clear from the discussion above, I would also add:

  • Move openfoam-solver to solvers/openfoam and check for similar instances in other tutorials (e.g., Quickstart)

I would indeed keep the FEniCS and Nutils as duplicate scripts, since this makes it easier for a user to extend/replace when preparing their own case.

Please upvote/downvote.

MakisH added a commit to precice/precice.github.io that referenced this issue Feb 7, 2024
As discussed in precice/tutorials#228 and partially implemented in precice/tutorials#459

Also updates minor aspects in the whole page (and introduces an intentional typo).
@MakisH
Copy link
Member

MakisH commented Feb 7, 2024

Documented the additional rules and guidelines in precice/precice.github.io#331

Overview of inconsistencies in #461

@MakisH MakisH closed this as completed Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants