bug fixes in ugwp and gsl drag suites #107
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Oct 13, 2021
|
@mdtoy Didn't we already merge this into the authoritative repositories? |
|
Yes, but it didn’t seem to make it into the NOAA-GSL repo.
… On Oct 19, 2021, at 2:04 PM, DomHeinzeller ***@***.***> wrote:
@mdtoy <https://github.com/mdtoy> Didn't we already merge this into the authoritative repositories?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMODLCIGOXYUZKFD54DCA3UHXFNPANCNFSM5F6H5V5A>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Are these commits cherry-picked from the authoritative repositories? |
|
No. I made a series of bug fixes in the ufs-weather-community/ufs-weather-model repo, which links to the NCAR/ccpp-physics repo over the past 6 months or so. It looked like the older bug fixes made it into the NOAA-GSL repo, but not these last three that I just submitted. The fix to drag_suite.F90 is the big one that fixed the “decomposition change” reproducibility.
… On Oct 19, 2021, at 2:12 PM, Dom Heinzeller ***@***.***> wrote:
Yes, but it didn’t seem to make it into the NOAA-GSL repo.
… <x-msg://14/#>
Are these commits cherry-picked from the authoritative repositories?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMODLDN36IBTABW6HT2KZ3UHXGJ5ANCNFSM5F6H5V5A>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
I am afraid we can't merge this. It will lead to lots of merge conflicts once we update gsl/develop from the authoritative repositories. There are two ways to do this:
The latter is obviously preferred, because it will have to happen anyway sooner or later, and the former would just be an unnecessary duplication of efforts. I started the update already, see the following draft PRs: NOAA-GSL/fv3atm#111 I expect these to go in immediately after Hannah's GF tuning PRs are merged. |
|
I thought I merged my bug fixes into the “top of the trunk” of the NOAA-GSL repo. When I issued the PR and set up the base repo settings, it said that the merges could be made.
… On Oct 19, 2021, at 3:31 PM, DomHeinzeller ***@***.***> wrote:
I am afraid we can't merge this. It will lead to lots of merge conflicts once we update gsl/develop from the authoritative repositories. There are two ways to do this:
Cherry pick the original commits to the authoritative repositories (if possible)
Update gsl/develop from the authoritative repositories
The latter is obviously preferred, because it will have to happen anyway sooner or later, and the former would just be an unnecessary duplication of efforts.
I started the update already, see the following draft PRs:
NOAA-GSL/fv3atm#111 <NOAA-GSL/fv3atm#111>
NOAA-GSL/ccpp-framework#19 <NOAA-GSL/ccpp-framework#19>
#109 <#109>
NOAA-GSL/ufs-weather-model#105 <NOAA-GSL/ufs-weather-model#105>
I expect these to go in immediately after Hannah's GF tuning PRs are merged.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMODLC277H7NT32VMGQXOTUHXPS7ANCNFSM5F6H5V5A>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
The point is that we need the *original* commits (hashes), not manual changes that are new commits hashes.
… On Oct 19, 2021, at 3:42 PM, mdtoy ***@***.***> wrote:
I thought I merged my bug fixes into the “top of the trunk” of the NOAA-GSL repo. When I issued the PR and set up the base repo settings, it said that the merges could be made.
> On Oct 19, 2021, at 3:31 PM, DomHeinzeller ***@***.***> wrote:
>
>
> I am afraid we can't merge this. It will lead to lots of merge conflicts once we update gsl/develop from the authoritative repositories. There are two ways to do this:
>
> Cherry pick the original commits to the authoritative repositories (if possible)
> Update gsl/develop from the authoritative repositories
> The latter is obviously preferred, because it will have to happen anyway sooner or later, and the former would just be an unnecessary duplication of efforts.
>
> I started the update already, see the following draft PRs:
>
> NOAA-GSL/fv3atm#111 <NOAA-GSL/fv3atm#111>
> NOAA-GSL/ccpp-framework#19 <NOAA-GSL/ccpp-framework#19>
> #109 <#109>
> NOAA-GSL/ufs-weather-model#105 <NOAA-GSL/ufs-weather-model#105>
> I expect these to go in immediately after Hannah's GF tuning PRs are merged.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMODLC277H7NT32VMGQXOTUHXPS7ANCNFSM5F6H5V5A>.
> Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5C2RJFAS3GRQK2JFM3FL3UHXQ4XANCNFSM5F6H5V5A>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Also, this PR won't be needed as soon as the PRs that I listed above are merged. |
|
By “original” commits, do you mean the commits I made in the NCAR/ccpp-physics repo (and that were merged)? It seems like that would be more of a spaghetti mess then the small manual deltas I made within the NOAA-GSL/ccpp-physics repo.
… On Oct 19, 2021, at 3:44 PM, Dom Heinzeller ***@***.***> wrote:
The point is that we need the *original* commits (hashes), not manual changes that are new commits hashes.
> On Oct 19, 2021, at 3:42 PM, mdtoy ***@***.***> wrote:
>
>
> I thought I merged my bug fixes into the “top of the trunk” of the NOAA-GSL repo. When I issued the PR and set up the base repo settings, it said that the merges could be made.
>
>
>
> > On Oct 19, 2021, at 3:31 PM, DomHeinzeller ***@***.***> wrote:
> >
> >
> > I am afraid we can't merge this. It will lead to lots of merge conflicts once we update gsl/develop from the authoritative repositories. There are two ways to do this:
> >
> > Cherry pick the original commits to the authoritative repositories (if possible)
> > Update gsl/develop from the authoritative repositories
> > The latter is obviously preferred, because it will have to happen anyway sooner or later, and the former would just be an unnecessary duplication of efforts.
> >
> > I started the update already, see the following draft PRs:
> >
> > NOAA-GSL/fv3atm#111 <NOAA-GSL/fv3atm#111>
> > NOAA-GSL/ccpp-framework#19 <NOAA-GSL/ccpp-framework#19>
> > #109 <#109>
> > NOAA-GSL/ufs-weather-model#105 <NOAA-GSL/ufs-weather-model#105>
> > I expect these to go in immediately after Hannah's GF tuning PRs are merged.
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMODLC277H7NT32VMGQXOTUHXPS7ANCNFSM5F6H5V5A>.
> > Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
> >
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5C2RJFAS3GRQK2JFM3FL3UHXQ4XANCNFSM5F6H5V5A>.
> Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMODLDJR6MWYPQEPXZAUMLUHXRCVANCNFSM5F6H5V5A>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
It won't be straightforward unless these commits were a few small and clean commits that only did what you are trying to do here. But again, we cannot merge new commits and then manually resolve all merge conflicts later. That is not how things work with git. |
|
Does it help that the three ccpp-physics files that are affected by this PR:
drag_suite.F90,
cires_ugwpv1_oro.F90,
and ugwpv1_gsldrag.F90
are identical to the ones in the NCAR/ccpp-physics repo? What I did, actually now that I remember, was just to copy them over from NCAR/ccpp-physics to my branch of NOAA-GSL/ccpp-physics. So effectively, they are already “merged" into the authoritative repository. Am I getting this right? If not, I’ll cry uncle. :)
… On Oct 19, 2021, at 3:53 PM, Dom Heinzeller ***@***.***> wrote:
By “original” commits, do you mean the commits I made in the NCAR/ccpp-physics repo (and that were merged)? It seems like that would be more of a spaghetti mess then the small manual deltas I made within the NOAA-GSL/ccpp-physics repo.
… <x-msg://22/#>
It won't be straightforward unless these commits were a few small and clean commits that only did what you are trying to do here. But again, we cannot merge new commits and then manually resolve all merge conflicts later. That is not how things work with git.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMODLAZKAYCUEU4FEIK7WLUHXSELANCNFSM5F6H5V5A>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
No need to cry ;-) they are identical but they are not merged, because a merge would mean that the commit history for these files is identical. In other words, after merging this PR, I would be able to merge ccpp-physics main as a whole and it would recognize that the changes had already been applied. This is not the case here, however, because GitHub doesn't inspect line by line differences and realizes that they are the same. It looks at the commit history. If the hash of a certain commit is not present, then it will try to apply it. Since you already applied it manually (in another commit), it will cause a merge conflict. |
|
Okay, that makes sense. Thanks for explaining.
… On Oct 19, 2021, at 4:59 PM, Dom Heinzeller ***@***.***> wrote:
Does it help that the three ccpp-physics files that are affected by this PR: drag_suite.F90, cires_ugwpv1_oro.F90, and ugwpv1_gsldrag.F90 are identical to the ones in the NCAR/ccpp-physics repo? What I did, actually now that I remember, was just to copy them over from NCAR/ccpp-physics to my branch of NOAA-GSL/ccpp-physics. So effectively, they are already “merged" into the authoritative repository. Am I getting this right? If not, I’ll cry uncle. :)
… <x-msg://24/#>
No need to cry ;-) they are identical but they are not merged, because a merge would mean that the commit history for these files is identical. In other words, after merging this PR, I would be able to merge ccpp-physics main as a whole and it would recognize that the changes had already been applied. This is not the case here, however, because GitHub doesn't inspect line by line differences and realizes that they are the same. It looks at the commit history. If the hash of a certain commit is not present, then it will try to apply it. Since you already applied it manually (in another commit), it will cause a merge conflict.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMODLCPTMQI5G74UY2MRADUHXZ4BANCNFSM5F6H5V5A>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
This PR is not necessary, because the updates will be brought over from the authoritative repositories in PR #109. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes 3 bugs in the ugwpv1/gsl drag suites:
drag_suite.F90: The modifications fix a failure to pass "decomposition change" regression tests. The "scale aware" variables "ls_taper" and "ss_taper" had been scalars based on the grid size at position "i=1" of the block, therefore, the results changed for each change in decomposition. Instead these variables are now 1D vectors that take into account the grid size at each grid point. There are other associated variables that changed from 1D vectors to 2D vectors as a result of the change, e.g., dxy4 and dxy4p. There were significant changes in loop structures as a result of this fix.
cires_ugwpv1_oro.F90: The calculation of the final u and v tendencies had been mixed up. This simple bug fix corrects the issue.
ugwpv1_gsldrag.F90: This fixes a logic error associated with the "do_ugwp" namelist switches.
The mods were regression-tested and the log file is on Hera at:
/scratch1/BMC/wrfruc/mtoy/git_local/ufs-weather-model.gsl_develop/tests/RegressionTests_hera.intel.m.log