Skip to content

Update w3srcemd.F90 to correct bug related to TR1#1384

Merged
sbanihash merged 1 commit into
NOAA-EMC:developfrom
Biao-Zhao:patch-2
Mar 17, 2025
Merged

Update w3srcemd.F90 to correct bug related to TR1#1384
sbanihash merged 1 commit into
NOAA-EMC:developfrom
Biao-Zhao:patch-2

Conversation

@Biao-Zhao
Copy link
Copy Markdown
Contributor

@Biao-Zhao Biao-Zhao commented Mar 4, 2025

To calculate the increment of spectrum, we should use the source term VSTR rather than VDTR

Pull Request Summary

Description

Issue(s) addressed

Commit Message

Check list

Testing

  • How were these changes tested?

  • Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)

  • Have the matrix regression tests been run (if yes, please note HPC and compiler)?

  • Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.)
    This PR corrects how the increment spectrum is calculated with TR1 switch cases, therefore answer changes are expected in any tests using that switch. Running matrix regression tests will add these two tests to the list of non-identical tests:
    ww3_tp1.9/./work_PR3_UQ_MPI (7 files differ)
    ww3_tp1.9/./work_PR3_UQ (7 files differ)

  • Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt)

To calculate the increment of spectrum, we should use the source term VSTR rather than VDTR
@sbanihash sbanihash self-requested a review March 4, 2025 16:18
@sbanihash sbanihash self-assigned this Mar 4, 2025
@Biao-Zhao Biao-Zhao changed the title Update w3srcemd.F90 Update w3srcemd.F90 to correct bug related to TR1 Mar 7, 2025
Copy link
Copy Markdown
Collaborator

@sbanihash sbanihash left a comment

Choose a reason for hiding this comment

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

Thank you @Biao-Zhao for your work on this fix. Everything looks good. I approve the changes.


********************* non-identical cases ****************************


mww3_test_03/./work_PR2_UNO_MPI_e (1 files differ)
mww3_test_03/./work_PR2_UQ_MPI_d2 (13 files differ)
mww3_test_03/./work_PR3_UQ_MPI_e_c (1 files differ)
mww3_test_03/./work_PR2_UNO_MPI_d2 (10 files differ)
mww3_test_03/./work_PR1_MPI_e (1 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c (15 files differ)
mww3_test_03/./work_PR3_UQ_MPI_e (1 files differ)
mww3_test_03/./work_PR2_UQ_MPI_e (1 files differ)
mww3_test_03/./work_PR3_UNO_MPI_e_c (1 files differ)
mww3_test_03/./work_PR1_MPI_d2 (8 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c (17 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2 (16 files differ)
mww3_test_03/./work_PR3_UNO_MPI_e (1 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2 (16 files differ)
mww3_test_09/./work_MPI_ASCII (0 files differ)
ww3_tp1.9/./work_PR3_UQ_MPI (7 files differ)
ww3_tp1.9/./work_PR3_UQ (7 files differ)
ww3_tp2.10/./work_MPI_OMPH (6 files differ)
ww3_tp2.16/./work_MPI_OMPH (4 files differ)
ww3_tp2.6/./work_ST4_ASCII (0 files differ)
ww3_ufs1.3/./work_a (3 files differ)


matrixCompSummary.txt
matrixCompFull.txt
matrixDiff.txt

@JessicaMeixner-NOAA
Copy link
Copy Markdown
Collaborator

Re-requesting a review from @sbanihash as I noticed that two tests:

ww3_tp1.9/./work_PR3_UQ_MPI (7 files differ)
ww3_tp1.9/./work_PR3_UQ (7 files differ)

that have unexpected changes.

@sbanihash
Copy link
Copy Markdown
Collaborator

sbanihash commented Mar 11, 2025

Thank you Jessica for pointing that out. I will take a look. ww3_tp1.9 is the only regtest that uses TR1 switch and change is expected based on the nature of this PR. I will do further checks to make sure they are expected. @Biao-Zhao have you done any testing with and without your changes to report any expected differences we will see?

@Biao-Zhao
Copy link
Copy Markdown
Contributor Author

Hi, @sbanihash, I didn't do any testing regarding this. I encountered this issue while going through w3srcemd.F90. Based on the treatment of other source terms, VSTR is expected to compute the contribution of TR1 to the increment of SPEC. Therefore, I think it's necessary to report this bug. If I got this wrong, feel free to point it out.

@sbanihash
Copy link
Copy Markdown
Collaborator

Thank you @Biao-Zhao for you response. I agree with the change and while I was reviewing your PR and looking at other source terms as you pointed out, this change was needed in my opinion as well. But as I had also mentioned to @JessicaMeixner-NOAA this PR will change answers for tests that use TR1 switch in the sense that it is intended to correct a mistake in calculating the increment spectrum.

@JessicaMeixner-NOAA
Copy link
Copy Markdown
Collaborator

If changes to tests with TR1 are expected, let's please update the PR template to separate the changes expected with this PR versus the regression tests that normally have b4b issues.

For example, the question to Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.) should be:

ww3_tp1.9/./work_PR3_UQ_MPI (7 files differ)
ww3_tp1.9/./work_PR3_UQ (7 files differ)

Copy link
Copy Markdown
Collaborator

@sbanihash sbanihash left a comment

Choose a reason for hiding this comment

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

@Biao-Zhao , @JessicaMeixner-NOAA PR description has been updated with the addition of the two test cases that are expected to change results with this fix. I approve the PR.

@Biao-Zhao
Copy link
Copy Markdown
Contributor Author

Thanks for the review and approval! @sbanihash @JessicaMeixner-NOAA. Looking forward to working together on more improvements. Keep up the great work!

@thesser1
Copy link
Copy Markdown
Collaborator

I checked through the pull request, given that the error came from the ERDC repo. I also approve this fix. Thanks for finding the bug.

@sbanihash sbanihash merged commit e0ceda9 into NOAA-EMC:develop Mar 17, 2025
@sbanihash
Copy link
Copy Markdown
Collaborator

Thank you @thesser1 for your review.

@Biao-Zhao Biao-Zhao deleted the patch-2 branch July 9, 2025 19:41
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