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

fix clock initialization for a restart #871

Merged

Conversation

pjpegion
Copy link

Pull Request Summary

This fixes issue #870

Description

Fixes the logic for a restart when there are not wave in the coupler and start_type needs to be set to initial.
Is a change of answers expected from this PR?

No changes to existing tests

Please also include the following information:

  • Add any suggestions for a reviewer
  • @DeniseWorthen @JessicaMeixner-NOAA @aliabdolali
  • Mention any labels that should be added: bug, documentation, enhancement, new feature
  • Are answer changes expected from this PR? Please describe the changes and the reason why in addition to which of the following labels would apply: mod_def change, out_grd change, out_pnt change, restart file change, Regression test
    -->

Issue(s) addressed

Commit Message

Check list

Testing

  • How were these changes tested?
    run the cpled_control_p8 and cpled_restart_p8 tests, and change
    start_type = continue to
    start_type = initial
    and the model now runs

  • Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)
    No, and I don't think we need to add a test.

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

  • Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.)
    I have tested cpld_control_p8 and cpld_restart_p8 and they both pass.

@MatthewMasarik-NOAA
Copy link
Collaborator

Thank you for finding and fixing this issue @pjpegion.

Note that the Github builds failed due to the known issue in the CI workflow for dev/ufs-weather-model.

@BrianCurtis-NOAA
Copy link

Before this can enter the UFSWM commit queue it needs a code review.

@JessicaMeixner-NOAA
Copy link
Collaborator

Before this can enter the UFSWM commit queue it needs a code review.

@BrianCurtis-NOAA This sounds really smart - but I thought the procedure before was to not approve these until tests at the ufs-weather-model tests have gone through, so this seems like a change so I wanted to confirm my understanding of this new paradigm.

Following up to review:
@pjpegion this PR looks good to me, but I know there's been lots of discussions about another update to this. Do we want to cherry-pick those changes in from @DeniseWorthen to this PR or move forward with this PR as is?

@BrianCurtis-NOAA
Copy link

@BrianCurtis-NOAA This sounds really smart - but I thought the procedure before was to not approve these until tests at the ufs-weather-model tests have gone through, so this seems like a change so I wanted to confirm my understanding of this new paradigm.

Yes, it seemed weird to me we would do all this testing without the actual CM's in the subcomponents reviewing the PR's. Also, some reviews from subcomponent PR's held up the commit queue, and we felt this change was beneficial to all. So this is a shift in our methodology. I see it as more control going to the subcomponents on how they want to run their PR structure and review process, and all we would need to look for from UFSWM would be review approvals and no changes from the most recent code review approval.

This is a work-in-motion, so if something works better for you, let me (us) know and we'll try to get it working.

@pjpegion
Copy link
Author

pjpegion commented Feb 3, 2023

@BrianCurtis-NOAA I am happy to hear this change in the order too. I always thought it was backwards to have code review after all of the tests, so if there was any changes due to code review, all of the tests would have to be re-run.

Denise proposed a cleaner solution that I have in this PR, but I don't know if the solution has been coded up.

@pjpegion
Copy link
Author

pjpegion commented Feb 3, 2023

I pulled in Denise's solution for the restart logic. My tests pass on hera.

@JessicaMeixner-NOAA
Copy link
Collaborator

@BrianCurtis-NOAA These changes are okay from the wave modeling code management side and I think this can be added to the ufs-weather-model queue. In the meantime, I have also requested reviews from @alperaltuntas and @DeniseWorthen for their awareness and for any feedback or reviews they'd like to give.

model/src/wav_comp_nuopc.F90 Outdated Show resolved Hide resolved
replace CESMFCOUPLED with W3_CESMCOUPLED
@MatthewMasarik-NOAA
Copy link
Collaborator

@pjpegion we discussed this PR briefly in UFS apps/comps meeting. The code review from WW3 mgrs is complete, and approved. I believe confirmation that no answers are changed is needed before being considered for the UFS commit queue. When you're able, please run the rt's and attach the log files to confirm this.

@DeniseWorthen
Copy link
Contributor

@MatthewMasarik-NOAA Actually This PR should not change baselines for UFS. What we need is for @pjpegion to attach the intel/gnu logs to the UWM PR to move it into the Q there.

@MatthewMasarik-NOAA
Copy link
Collaborator

@MatthewMasarik-NOAA Actually This PR should not change baselines for UFS. What we need is for @pjpegion to attach the intel/gnu logs to the UWM PR to move it into the Q there.

@DeniseWorthen thank you for the clarification.

@JessicaMeixner-NOAA JessicaMeixner-NOAA merged commit af544dc into NOAA-EMC:dev/ufs-weather-model Mar 14, 2023
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.

6 participants