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

Fixed subcycling in implicit coupling (for regular simulations) #106

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

boris-martin
Copy link
Collaborator

@boris-martin boris-martin commented Sep 25, 2022

(Depends on #105 )

Fixes #9. Essentially, checkpointing stores more data than previously (not only positions, but velocities and a few more things), because those additional fields must be stored when going back from more than an increment (time step). So previous implementation worked only when there was one step per window.

Still WIP:

  • Refactoring & cleaning
  • Outputting once per time step instead of once per window (using the buffer from Fixed subcycling in implicit modal dynamic simulations #105)
  • Allow user-defined output frequency (once every N steps)
  • Testing on more cases, including thermal cases (only perpendicular flap so far)
  • Check for redundancy (I may have copied more data than necessary?)

@boris-martin boris-martin marked this pull request as ready for review September 26, 2022 13:30
@boris-martin
Copy link
Collaborator Author

Turns out adapting the buffer code is trickier than anticipated. I'm undrafting and I suggest staying on the current partial fix: correct results, but with one output every N time window instead of every N steps, with N defaulting to 1.
This will limit the introduction of new bugs for now.

Copy link
Contributor

@KyleDavisSA KyleDavisSA left a comment

Choose a reason for hiding this comment

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

Changes look good. All new variables are freed in the end. However the perpendicular flap does not function correctly.

  • develop branch with equal time steps functions correctly, but breaks for subcycling.
  • with this PR, subcycling and equal time steps do not crash, but the flap hardly moves.

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 tried running the perpendicular flap, but it looks like we need to consider updates based for v2.20:

  • With v2.19 and this PR, the tutorial was running normally
  • With v2.20, it is running normally
  • With v2.20 and this PR, the flap does not move

Please also check if it actually fixes the original issue (example on how to reproduce it).

@KyleDavisSA
Copy link
Contributor

My suggestion would be to continue with the v2.20 release and continue to work on subcycling. Rather have the adapter working correctly when all time steps are equal.

@KyleDavisSA KyleDavisSA self-requested a review November 16, 2022 13:39
@boris-martin
Copy link
Collaborator Author

I tried running the perpendicular flap, but it looks like we need to consider updates based for v2.20:

* With v2.19 and this PR, the tutorial was running normally

* With v2.20, it is running normally

* With v2.20 and this PR, the flap does not move

Please also check if it actually fixes the original issue (example on how to reproduce it).

How did you build "This + 2.20 ?" Since this PR involves quite some modifications of the main file I planned to do it by hand

@MakisH
Copy link
Member

MakisH commented Nov 16, 2022

I tried running the perpendicular flap, but it looks like we need to consider updates based for v2.20:

* With v2.19 and this PR, the tutorial was running normally

* With v2.20, it is running normally

* With v2.20 and this PR, the flap does not move

Please also check if it actually fixes the original issue (example on how to reproduce it).

How did you build "This + 2.20 ?" Since this PR involves quite some modifications of the main file I planned to do it by hand

I checked out the v2.20 branch and I merged this branch, which did not lead to any conflicts. I guess we do need to update things, but let's make that a bugfix release, then. There is no regression.

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.

4 participants