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

Change exchanged data DisplacementDelta to Displacement in elastic-tube-3d #255

Closed
wants to merge 4 commits into from

Conversation

IshaanDesai
Copy link
Member

This PR changes the data exchanged in the elastic-tube-3d case from DisplacementDelta to Displacement. This is mainly done so that FEniCS can be added as a participant to the elastic-tube-3d case. The FEniCS FSI code used in all the tutorials computes in terms of absolute displacement and hence it was straightforward to develop the code again.

The FEniCS participant is added via #222

Changing the exchange data has no effect on the simulation results as both the openfoam-adapter and calculix-adapter are capable of handling absolute displacements for FSI cases.

@IshaanDesai IshaanDesai self-assigned this Feb 3, 2022
@IshaanDesai IshaanDesai marked this pull request as draft February 3, 2022 12:04
@IshaanDesai IshaanDesai marked this pull request as ready for review February 3, 2022 12:12
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

If the case still works, I'm fine with the name change. I'm just a bit unsure about what is actually happening here: Are the solvers really computing and exchanging absolute displacements now? How do you tell both solvers/adapters to do so? Is just the keyword Delta in the data name responsible for this (quite large) change in functionality? Or did we maybe always exchange absolute displacements and never relative?

Sorry for all the questions, I'm just confused how a diff which is only replacing names with other names can have the described impact.

@IshaanDesai
Copy link
Member Author

At least for the CalculiX adapter I know that a simple name change is sufficient to really change what quantity is being used. So now the adapter is indeed working with absolute displacements. I imagine that something similar is implemented in the OpenFOAM adapter.

@MakisH
Copy link
Member

MakisH commented Feb 3, 2022

@BenjaminRodenberg Indeed, the name is directly related to the type. While this restricts the naming possibilities, it is implemented this way to avoid additional complexity in the adapter config (at least in the OpenFOAM adapter, I have tried adding such a feature, but it was quickly getting very complex).

Changing the exchange data has no effect on the simulation results

@IshaanDesai I would be happy to believe that, but since this is directly changing the coupling variables, some evidence would be welcome. 😅

One concern to consider in the tradeoff is that, if we remove DisplacementDelta from here, then we will have no test case for this feature, which has already reported some issues:
precice/calculix-adapter#8

It looks like we currently don't mention the coupling data in the documentation, so there is nothing to update.

@BenjaminRodenberg
Copy link
Member

One concern to consider in the tradeoff is that, if we remove DisplacementDelta from here, then we will have no test case for this feature, which has already reported some issues:
precice/calculix-adapter#8

I'm currently a bit worried about the overall direction this PR is heading into. As far as I understand, moving from relative to absolute displacement is a big step for all cases and might introduce errors. We would additionally miss an important test case for the OpenFOAM and CalculiX adapter. Is it possible to implement the FEniCS-OpenFOAM coupling in a way that does not touch the way the FEniCS-CalculiX coupling works? I can imagine the following possible solutions:

  • Add a feature in directly preCICE to "translate" from relative data to absolute data (displacement or something else). Imagine one solver+adapter can only provide absolute, the other can only provide relative. Then there just is no way to couple them at the moment. This is probably a bigger feature.
  • Provide two different configurations. One with relative displacement for OpenFOAM-CalculiX. Another one with absolute for OpenFOAM-FEniCS. We would then at least restrict the impact of adding the FEniCS case to only that specific solver setup and not to the whole example. We would also keep the OpenFOAM-CalculiX case. This might be a possible work-around.
  • Add support for DisplacementDeltas in FEniCS adapter. What I do not really like about this solution: We have to do this for every adapter+solver again and again. I am also not a big fan of the fact that just by changing the name the functionality changes. As explained above: There are technical reasons to do this in the OpenFOAM and CalculiX adapter and I cannot comment on this, but I am not so convinced that this is a feature we want to implement in the FEniCS adapter.

@IshaanDesai
Copy link
Member Author

I thought more on this change and changed my mind. Lets not change the existing OpenFOAM - CalculiX case just to fit the FEniCS case into the whole setup. Instead I tried to modify the FEniCS setup to work with relative displacements, and I think it somehow works: 891ee05 I will test this further.

I am closing this PR as this change is not in the right direction.

@IshaanDesai IshaanDesai closed this Feb 4, 2022
@MakisH
Copy link
Member

MakisH commented Feb 4, 2022

Add a feature in directly preCICE to "translate" from relative data to absolute data (displacement or something else). Imagine one solver+adapter can only provide absolute, the other can only provide relative. Then there just is no way to couple them at the moment. This is probably a bigger feature.

This sounds like a nice idea that should be documented in an issue in the preCICE repository.

Add support for DisplacementDeltas in FEniCS adapter.

Just to clarify: The OpenFOAM and CalculiX adapters support both absolute (Displacement) and relative (DisplacementDelta) displacements. Adding this in FEniCS as an option would just complete the puzzle.

If we absolutely need this case with FEniCS and we absolutely need it with absolute displacements, then we can discuss other solutions. But there should be a clear motivation for this path.

@IshaanDesai
Copy link
Member Author

Just to clarify: The OpenFOAM and CalculiX adapters support both absolute (Displacement) and relative (DisplacementDelta) displacements. Adding this in FEniCS as an option would just complete the puzzle.

The FEniCS adapter is a bit more abstract in the sense of identifying quantities that it deals with. The choice of absolute vs. relative quantities would go into the FEniCS script that the user writes.

If we absolutely need this case with FEniCS and we absolutely need it with absolute displacements, then we can discuss other solutions. But there should be a clear motivation for this path.

I can say with absolute certainty that we absolutely do not need to have absolute displacements in FEniCS.

@BenjaminRodenberg
Copy link
Member

Add a feature in directly preCICE to "translate" from relative data to absolute data (displacement or something else). Imagine one solver+adapter can only provide absolute, the other can only provide relative. Then there just is no way to couple them at the moment. This is probably a bigger feature.

This sounds like a nice idea that should be documented in an issue in the preCICE repository.

Done. precice/precice#1173

@MakisH MakisH added this to the v202104.2.0 milestone Feb 9, 2022
@IshaanDesai IshaanDesai deleted the IshaanDesai-patch-1 branch November 14, 2022 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants