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

def/ufs: Bug fix to avoid accessing uninitialized date #1112

Conversation

JessicaMeixner-NOAA
Copy link
Collaborator

Pull Request Summary

This PR addresses a bug that was discovered on hercules with gnu debug compilers, where an uninitialized date was being used.

Description

This changes logic in w3wave to avoid calling a function with an uninitialized date. This bug has existed for quite a while but only was exposed on a particular platform with debug in gnu which gave a small-ish random value. Intel tends to initialize to 0 and extremely large random numbers did not cause issues because flout(7) was also false and we didn't seem to try to access a variable with month 52 that way.

Please also include the following information:

  • Add any suggestions for a reviewer @MatthewMasarik-NOAA
  • Mention any labels that should be added: bug
  • Are answer changes expected from this PR? No changes are expected.

Issue(s) addressed

This partially address issues #1109 (also needs PR to develop to be closed) and partially addresses ufs-community/ufs-weather-model#1963

Commit Message

def/ufs: Bug fix to avoid accessing uninitialized date

Check list

Testing

  • How were these changes tested? On hera in ufs-weather-model a baseline was created, WW3 was updated with a bug fix and the baselines were matched. Testing on hercules resulted in other errors and will be fully tested in ufs-waether-model regression test/commit process.
  • Are the changes covered by regression tests? (If not, why? Do new tests need to be added?) yes-ish, exposed only in debug mode on one platform
  • Have the matrix regression tests been run (if yes, please note HPC and compiler)? No - ufs-weather-model tests were run instead, a PR to develop branch will have matrix regression tests
  • Please indicate the expected changes in the regression test output, -- this should not change answers.

@MatthewMasarik-NOAA
Copy link
Collaborator

Thanks, @JessicaMeixner-NOAA! I'll start the tests now.

@MatthewMasarik-NOAA
Copy link
Collaborator

Woops, just a clarification: this is going into dev/ufs-weather-model, so no tests to run for this branch. Reviewing now.

Copy link
Collaborator

@MatthewMasarik-NOAA MatthewMasarik-NOAA left a comment

Choose a reason for hiding this comment

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

Code review

Pass

Testing

Pass

  • branch: dev/ufs-weather-model
    • UFS RT - Author has confirmed RTs created/matched a baseline.
    • WW3 regtests - N/A (matrix regtests are only run for develop).

Approved.

@MatthewMasarik-NOAA MatthewMasarik-NOAA merged commit 02693d8 into NOAA-EMC:dev/ufs-weather-model Oct 26, 2023
20 checks passed
@MatthewMasarik-NOAA
Copy link
Collaborator

@JessicaMeixner-NOAA thank you for the very quick detective work and fix submission! It is great to have addressed a longstanding, hidden bug in WW3!

@JessicaMeixner-NOAA
Copy link
Collaborator Author

@MatthewMasarik-NOAA it's not time ot merge this in this branch. we do not merge until it's turn in the ufs-weathe-rmodel queue

Can we unmerge this?

@MatthewMasarik-NOAA
Copy link
Collaborator

@MatthewMasarik-NOAA it's not time ot merge this in this branch. we do not merge until it's turn in the ufs-weathe-rmodel queue

Can we unmerge this?

Right. My mistake. Yes, I'll work on it now.

@JessicaMeixner-NOAA JessicaMeixner-NOAA deleted the bug/devufs/flout7uninit branch December 12, 2023 21:26
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