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

Several corrections to the coherency checks. #759

Merged
merged 1 commit into from
Sep 6, 2022

Conversation

MathieuDutSik
Copy link
Contributor

@MathieuDutSik MathieuDutSik commented Aug 13, 2022

Pull Request Summary

Simplifying and correcting debugging statements.

Description

Simplify and corrects a few parts of the coherency checks.
(like identifying VA(0:NSEAL) with FIELD(1:NSEAL) which is problematic and cause memory problems).

I thus believe it is useful to merge as it helps debugging and has zero impact on normal use.

Issue(s) addressed

The DEBUG statements are a very useful feature of the WW3 system. I thus believe
that it is very important to have correct debugging feature that do not lead to the wrong
idea about errors.

Commit Message

Simplifying and correcting debugging statements

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.)
  • Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):

@aliabdolali
Copy link
Contributor

This PR simplifies and corrects a few parts of the coherency checks (like identifying VA(0:NSEAL) with FIELD(1:NSEAL) which is problematic).

I thus believe it is useful to merge as it helps debugging and has zero impact on normal use.

Hi @MathieuDutSik Thanks for providing this development. I'd appreciate it if you provide an extended description of why you did this and how you expect it would benefit WW3. Could you fill out the PR template completely? If some of them are not applicable, you can leave NA in front.

@JessicaMeixner-NOAA
Copy link
Collaborator

@MathieuDutSik thanks for updating more of the descriptions. Would it be okay if the eventual commit message is "Simplifying and correcting debugging statements" as I think it might be more informative than "sync simplify_logs with develop"? Can you address the checklist and provide any information about testing. Thank you so much for this update! I'm testing two other PRs right now and then will run regression tests for this PR.

@MathieuDutSik
Copy link
Contributor Author

@MathieuDutSik thanks for updating more of the descriptions. Would it be okay if the eventual commit message is "Simplifying and correcting debugging statements" as I think it might be more informative than "sync simplify_logs with develop"? Can you address the checklist and provide any information about testing. Thank you so much for this update! I'm testing two other PRs right now and then will run regression tests for this PR.

Ok, I did a renaming of the "eventual commit message".
Regarding the checklist, I do not understand what I should do because some merged PRs like #714 have not the checklist done.
So, I would like things to be clarified on that.

@JessicaMeixner-NOAA
Copy link
Collaborator

@MathieuDutSik the checklist is simply a quick list of reminders for developers as they submit the PR. For example, if your branch is up to date with the develop branch of WW3, you would check the box (or in the edit mode its [x]). For the testing, we'll do testing on our end as well, but we'd like to hear how this PR was tested by the developer submitting the PR.

Our HPC is very full right now, but I will be running the regtests for this PR and completing the review of this PR this week. I apologize for the delay and long queue time for this PR. We will work to reduce this time in the queue in the future. I appreciate your patience with us and most importantly we appreciate your contributions to WW3!

@JessicaMeixner-NOAA
Copy link
Collaborator

@MathieuDutSik I just wanted to let you know that I ran the regtests for this PR, but it was on a machine that disk is having some issues, so I'm having to re-run them now on another machine. I'm also in the process of finalizing the review of PR741. I don't think I"m going to be able to merge this PR this week as I said 2 days ago and Monday is a holiday, so my best guess of a timeline is Tuesday or Wednesday of next week. I've reviewed the code itself and do not see any issues there. Thanks again for your patience!

@MathieuDutSik
Copy link
Contributor Author

Thank you for your answer. Let me know how I can help you.
As you have seen, there is plenty of further corrections down the line, but I am waiting for this one to go through before writing them.

@JessicaMeixner-NOAA
Copy link
Collaborator

Only the expected cases have issues:

**********************************************************************
********************* non-identical cases ****************************
**********************************************************************
mww3_test_03/./work_PR2_UNO_MPI_d2                     (12 files differ)
mww3_test_03/./work_PR1_MPI_d2                     (14 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c                     (14 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c                     (15 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2                     (17 files differ)
mww3_test_03/./work_PR2_UQ_MPI_d2                     (15 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2                     (15 files differ)
ww3_ta1/./work_UPD0F_U                     (0 files differ)
ww3_tp2.10/./work_MPI_OMPH                     (7 files differ)
ww3_tp2.16/./work_MPI_OMPH                     (4 files differ)
ww3_ufs1.3/./work_a                     (1 files differ)

matrixCompFull.txt
matrixCompSummary.txt
matrixDiff.txt

Copy link
Collaborator

@JessicaMeixner-NOAA JessicaMeixner-NOAA left a comment

Choose a reason for hiding this comment

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

@MathieuDutSik thank you for your updates! Looking forward to the next set of updates!

@JessicaMeixner-NOAA JessicaMeixner-NOAA merged commit 710b8bb into NOAA-EMC:develop Sep 6, 2022
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