-
Notifications
You must be signed in to change notification settings - Fork 51
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
Application fixes #80
base: main
Are you sure you want to change the base?
Conversation
eef0694
to
a64075d
Compare
@jtgrasb the moordyn test is now passing here too! The mooring matrix case is what fails the mooring test now |
I ultimately chose to fix the passive yaw test by replacing the results that our regression test compares against because:
@dforbush2 Let me know if you disagree with this approach |
I do not disagree. I opted for spline in this case because it allows the specification of the extrapolated value (or something like that?) that linear does not...and I would like to have the ability to consider a fixed/zero'd out set of hydrodynamic data beyond a certain extent. This facilitates runs where the range of motion is known a priori by making the required range of BEM data smaller.
From: Adam Keester ***@***.***>
Sent: Thursday, January 23, 2025 2:32 PM
To: WEC-Sim/WEC-Sim_Applications ***@***.***>
Cc: Forbush, Dominic Dean ***@***.***>; Mention ***@***.***>
Subject: [EXTERNAL] Re: [WEC-Sim/WEC-Sim_Applications] Application fixes (PR #80)
I ultimately chose to fix the passive yaw test by replacing the results that our regression test compares against because:
* the passive yaw results are identical when using the previous linear interpolation instead of the current spline interpolation.
* the current difference in results is extremely small. The regression test currently only allows a zero tolerance, which is likely more stringent than is warranted.
* The spline vs linear interpolation in bodyClass.irrExcitation is the only source of difference, and we could argue this method is more accurate to interpolate against anyways
@dforbush2<https://github.com/dforbush2> Let me know if you disagree with this approach
-
Reply to this email directly, view it on GitHub<#80 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AHF2G7HZW5D3XPI6FBWOGK32MFNT7AVCNFSM6AAAAABVKKLMD6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMJRGA2TQMJUGU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@kmruehl assigning you for now since WEC-Sim/WEC-Sim#1401 is also assigned to you and it's for the same effort to resolve application bugs |
@kmruehl this is ready for review |
@kmruehl any updates on the review for this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks good. I'm waiting for the tests to pass to merge.
@akeeste Mooring and Paraview tests are not yet passing, please work with @dforbush2 to resolve while I'm out. |
The moordyn tests (mooring/moordyn and paraview/RM3) both fail sporadically, crashing MATLAB. The error is unintelligible and cannot be produced. Tests pass locally so I propose we just remove them and revisit those tests at a different time. The Paraview/OSWEC accelerator build fails for some reason. I will dig into this again and hopefully get these tests finally working |
@kmruehl what was the problem with the Paraview OSWEC slx file? It passed on R2023b but is failing now on R2024b. I get an warning that blocks were removed when it was exported from R2024a to R2023b (though it doesn't end up showing any for which that was the case). I don't have R2024b to test with. |
@dforbush2 @kmruehl The runner is not working robustly. Paraview OSWEC works locally with R2024b. I propose we skip both paraview and moordyn tests for now so that we can finish the progress made here and move on. |
Tied to and should be merged after WEC-Sim/WEC-Sim#1401.
Fixes WEC-Sim/WEC-Sim#1400