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

Time integrators #182

Merged
merged 73 commits into from
Jun 27, 2024
Merged

Conversation

sanguinariojoe
Copy link
Collaborator

@sanguinariojoe sanguinariojoe commented Jan 14, 2024

THIS PR DEPENDS ON #224

Well, I am afraid this will require quite a bit of discussion on several of the changes I implemented:

  • Fixed the Adams-Bashforth schemes here. I suppose this does not require any kind of discussion at all.
  • Added a CFL (Courant–Friedrichs–Lewy) option, in a such a way the user can either define the time step (dtM) or the CFL factor (cfl). He can even define both, so the most restrictive is considered.
  • Removed the Z presevation check on the catenary solver (see here). To be honest, I do not remember if we added that, or it was always there. Anyway, if we want that back, we need to use the relative error instead. Otherwise, the errors for big depths will fall inside that condition (600 meters seems enough)
  • Seabed is only considered now on the catenary solver when one of the line-ends is close enough to the bottom. This fix the initialization in many situations, and I do not think it is worsening the results on any context.
  • Added Local-timestep versions of the Euler and the Adams-Bashforth time schemes. Tests on the lEuler have been created
  • I substituted the previous initial condition by a stationary solver, which is essentially the same than before but with an infinitely large dissipation (indeed the trick is that velocity is completely removed). The main benefit is that the new solver is unconditionally stable (with the booster it might get unstable for a while though). Some other benefits are that it is able to converge to a way more precise solution. On the other hand, the null velocity might means that the solution would take significantly longer to be reached, specially when the catenary solution fails. Anyway, since the user still have the possibility of saving and loading, I do not see much benefit on the previous approach, which can still be used anyway.

@sanguinariojoe
Copy link
Collaborator Author

Hey guys! I uploaded a Newmark scheme. An Average Constant Acceleration (ACA) one to be more specific, with relaxation, which is able to remain stable with significantly larger time steps.

@mattEhall and @RyanDavies19 , it would be great if you can review this and give feedback.

@RyanDavies19
Copy link
Collaborator

@sanguinariojoe just keeping you in the loop, Matt and I have both been pretty busy with other work so we haven't gotten the chance to dig too much into this yet. It's still on our radar, and hopefully I'll be able to get to it soon. Currently I am working on adding vortex induced vibrations to MoorDyn.

@sanguinariojoe
Copy link
Collaborator Author

Currently I am working on adding vortex induced vibrations to MoorDyn

Cool! I was told to avoid this on the past because apparently those are engineered out, so our digital twins shall not take them into account

@sanguinariojoe
Copy link
Collaborator Author

Hey @RyanDavies19 !

This branch is evolving quite much... I am indeed never using dev branch anymore.

  • The stationary solution is way more powerful than the previous Drag increased Initial Condition generator. Probably there is room for even further improvements.
  • The implicit schemes (specially the midpoint) are clearly outperforming the explicit ones.
  • I added auto-mem-checks, which are really handy.
  • I fixed several errors

I really think we should start addressing this before both branches start diverging too much

@RyanDavies19
Copy link
Collaborator

@sanguinariojoe agreed. I will be reviewing today now that I have some time available for MDC work. Thank you for including the docs, that is very helpful. My biggest concerns are making sure that 1) we retain backwards compatibility in the form of input files, and 2) the new input file is well documented (which it seems you've already done a lot of). Many of the questions I see in the OpenFAST issues/forum are from confusions between the two different code versions so keeping things clear will be important.

@sanguinariojoe
Copy link
Collaborator Author

sanguinariojoe commented Jun 13, 2024 via email

@RyanDavies19
Copy link
Collaborator

RyanDavies19 commented Jun 13, 2024

@sanguinariojoe okay that would be good. I'll add comments in the code where some slight changes might be needed. Once you get those docs done it would be good to read

docs/inputs.rst Outdated Show resolved Hide resolved
docs/tschemes.rst Outdated Show resolved Hide resolved
@sanguinariojoe
Copy link
Collaborator Author

I rebased this PR on #224, so that PR shall be merged first

@RyanDavies19
Copy link
Collaborator

@sanguinariojoe there should be a PR up to OpenFAST/dev tomorrow afternoon with updated reg tests for comparison using #224

@sanguinariojoe
Copy link
Collaborator Author

sanguinariojoe commented Jun 24, 2024 via email

@RyanDavies19
Copy link
Collaborator

RyanDavies19 commented Jun 24, 2024

@sanguinariojoe I agree it looks good to go. OpenFAST dev is a few weeks away from merging into main so we should be able to get the new reg tests in by then. If not it's a simple fix to point to the dev branch. Only remaining question is the GH artifact comment I made on #224. It would be good to have comparison plots if the test fails

@RyanDavies19
Copy link
Collaborator

@sanguinariojoe Added a few more docs reviews. I will open a PR to your branch with the fixes. Overall though they look really good and are very helpful. Thanks for putting them together!

@sanguinariojoe
Copy link
Collaborator Author

sanguinariojoe commented Jun 26, 2024

See OpenFAST/r-test#127 for updated reg tests

Hey @RyanDavies19 , the new tests do not work to me... In OpenFAST I mean. It seems that all the drivers named like BodiesAndRods_driver.dvr shall be replaced by md_driver.inp

After making all those replacements, it complains about the lack of the reference results. For instance:

131: Error: Error: file does not exist at "/home/pepe/CoreMarine/openfast.action/openfast/reg_tests/../reg_tests/r-test/modules/moordyn/md_BodiesAndRods/driver.MD.out"

Finally, several tests are not printing any output to compare with, so the driver.MD.out will be empty.

I suggest merging this and we can carry out the verifications when they are ready. dev branch is precisely meant for that

@RyanDavies19
Copy link
Collaborator

@sanguinariojoe Agreed lets merge. I talked with Matt and that's fine. Apologies for confusion with OpenFAST/r-test#127, I should have opened that as a draft. It is a work in progress.

@sanguinariojoe sanguinariojoe merged commit c0b5b67 into FloatingArrayDesign:dev Jun 27, 2024
7 checks passed
@sanguinariojoe
Copy link
Collaborator Author

Hurray! Thanks guys!

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.

2 participants