Skip to content

Conversation

@MShabara
Copy link
Contributor

This PR modifies the MOST example files to enable the second order forces calculation.
The PR to be reviewed after merging PR 1472

@jtgrasb
Copy link
Contributor

jtgrasb commented May 16, 2025

@MShabara I tried running this case to test it out, but it looks like the QTFs aren't included in the BEM results from NEMOH. Based on our readNEMOH.m script, there should also be a results/QTF folder that includes the QTFs.

Also, since you are changing the case slightly by adding the QTFs, we expect that the results should change slightly. We will need to update the original data that the regression test compares to.

@jtgrasb
Copy link
Contributor

jtgrasb commented May 21, 2025

@MShabara Just tested this out and it works now. Looking at the failing tests, it does look like the results are just changed slightly by the addition of QTFs. The original data that its comparing to can be updated to reflect the addition of QTFs and should pass once updated.

image

@MShabara
Copy link
Contributor Author

@jtgrasb, to avoid the influence of the QTF on both test cases, I’m limiting the tests to the Constant case only for now.
These tests require the Optimization Toolbox that I don't have, so I'm running them via GitHub Actions. However, there's been a persistent issue over the past 3 days affecting MATLAB's GitHub integration:
🔗 MathWorks status page
🔗 Related GitHub issue

I'll resume full testing once the issue is resolved.

@kmruehl kmruehl requested a review from jniffene May 28, 2025 14:55
@kmruehl kmruehl requested a review from jtgrasb July 15, 2025 22:19
@kmruehl kmruehl assigned jtgrasb and unassigned jniffene Jul 15, 2025
@jtgrasb
Copy link
Contributor

jtgrasb commented Jul 15, 2025

@MShabara I pushed to this PR to force the tests to rerun. A couple of notes:

  • Remove gbmWEC-Sim.mat - I don't think this was intentionally added.
  • It looks like the tests are passing for the constant case but failing for the turbulent case. So, there may be an issue with the MOST tests themselves since the turbulent case isn't testing QTFs.
    • In terms of the tests, let's get #1477 merged first, then update the data we're comparing to with QTFs added.

MShabara added 2 commits July 28, 2025 12:20
disables the QTF calculation in the example case
@MShabara
Copy link
Contributor Author

@jtgrasb The QTFs were disabled for this PR.
The tests are failing, but PR 95 will handle this issue. If this look good to you, you can go ahead and merge the PR

@jtgrasb
Copy link
Contributor

jtgrasb commented Jul 30, 2025

Looks good, merging now.

@jtgrasb jtgrasb merged commit 80c7c31 into WEC-Sim:dev Jul 30, 2025
1 of 3 checks passed
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.

3 participants