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

Add a fluid-fake for easier debugging #472

Merged

Conversation

BenjaminRodenberg
Copy link
Member

See #176 for motivation. Here is a small drawing:

Screenshot from 2024-02-29 11-42-37

@MakisH
Copy link
Member

MakisH commented Mar 11, 2024

I am struggling to decide if we should pursue this direction or not.

On the one hand:

  • it is clear that it was useful for you
  • this is what we were sometimes suggesting users to try out first
  • it does not seem to harm merging it
  • it sounds like the right tutorial to add such a fake solver, since most of our users probably start from this tutorial. But let's not add fake solvers to more tutorials.

I also understand that the motivation was mainly to have uni-directional coupling instead of bi-directional, to not have to feed back data while developing the FEniCS adapter.

On the other hand, this is why we developed the ASTE replay mode. Instead of creating fake solvers, we could instead (in the long-term) generate replay data and store them next to the reference-results we now have.

Clearly, the situation is "I developed a quick solution before the long-term solution was available, do we now also want to make the quick solution available, or not?". Given that ASTE is currently not super easy to install, and this is probably more intuitive for users, I would say "why not". But let's add a note in the README that ASTE replay mode would be the more powerful approach.

@uekerman uekerman marked this pull request as ready for review March 11, 2024 13:20
@BenjaminRodenberg
Copy link
Member Author

BenjaminRodenberg commented Apr 2, 2024

Since I stumbled across this PR and I think we should merge it rather sooner than later:

But let's add a note in the README that ASTE replay mode would be the more powerful approach.

I can link to https://precice.org/tooling-aste.html.

However, I do not fully agree that both are addressing the same thing: The replay mode has a different use case.

With the fake fluid you replace the real fluid with a simplified fake version. This eliminates possible sources of error (other solver and other adapter) and allows us to focus on only one party in the overall simulation (this solver and this adapter). I think this is useful, if you do not want to bother with the other solver or you do not trust or cannot assess the output. This is particularly useful for development when the other side is not yet available.

The replay mode only bypasses the computation of one solver and simplifies the feedback loop somehow. This reduces computation time and provides an environment that is close to reality. But you should be quite sure that the original setup that you used for creating the recording is correct. This is useful for detecting regressions.

Note: I'm planning to write something about this whole topic in my thesis. This should serve as a proper documentation as soon as published. Until then I would rather not spend time coming up with more documentation pages here.

@uekerman
Copy link
Member

uekerman commented Apr 2, 2024

The replay mode only bypasses the computation of one solver and simplifies the feedback loop somehow. This reduces computation time and provides an environment that is close to reality. But you should be quite sure that the original setup that you used for creating the recording is correct. This is useful for detecting regressions.

With ASTE, you could also write artificial data. Then, you have exactly the same functionality as the fake-fluid here.

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.

Let's add this.

Could be good to also add a requirements.txt and a venv here. Similar to the Nutils cases.

perpendicular-flap/README.md Outdated Show resolved Hide resolved
perpendicular-flap/fluid-fake/fake.py Show resolved Hide resolved
@BenjaminRodenberg
Copy link
Member Author

Ready to merge. @MakisH I'll wait for your ok or you can also push the button, if you want. Don't want to interfere with your release with something that is not critical.

Copy link
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

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

I think that the current state is nice and sustainable. There is a clear description in the README, there is a clear hint to ASTE (while it is also implied that ASTE would be more complicated to use), and the case follows the guidelines.

I also tested the following combinations, and they completed, resulting into a reasonably moving flap:

  • fake-deal.II
  • fake-OpenFOAM
  • fake-Nutils

Let's merge this already, there is no reason to wait, and I can add the watchpoint next to the rest.

perpendicular-flap/README.md Outdated Show resolved Hide resolved
@MakisH MakisH merged commit bb07190 into precice:develop Apr 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants