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

Pull in changes from feature/sdrabenh/r21c_G2G #735

Merged
merged 22 commits into from
Apr 19, 2023

Conversation

sdrabenh
Copy link
Collaborator

@sdrabenh sdrabenh commented Apr 5, 2023

No description provided.

Amal El Akkraoui and others added 18 commits April 28, 2022 15:12
…Forcing

Modified observed precip regridding to use conservative algorithm.
…tional minor fixes to long names in Surface GC and Catch GC
@sdrabenh sdrabenh added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Apr 5, 2023
@sdrabenh sdrabenh requested a review from mathomp4 April 5, 2023 19:03
@sdrabenh sdrabenh requested review from a team as code owners April 5, 2023 19:03
sanAkel
sanAkel previously approved these changes Apr 5, 2023
zhaobin74
zhaobin74 previously approved these changes Apr 5, 2023
@gmao-rreichle
Copy link
Contributor

@sdrabenh, I'm afraid there's an issue with CatchCN in your branch. After a very quick look, here's what I think happened. I might be wrong, but what I'm seeing is concerning enough to throw a flag.
In a nutshell, #637 was on a rather old branch of the GCM GC that still had only one GC F90 file for CatchCNCLM40 and CatchCNCLM45. At some time between then and now, CatchCN was restructured, and lot of the code in the CatchCN GC file moved into subdirs and GC F90 files specific to CatchCNCLM40 and CatchCNCLM45. The problem is that when you went from your branch feature/rreichle/r21c_updates (associated with #637) to feature/sdrabenh/r21c_G2G (associated with the present PR), the CN-related changes from #637 got lost.
We'll need to take a closer look and, if I'm not mistaken, figure out the best way to resurrect the changes of #637 that fell through the cracks. For now, I'm holding off on hitting "approve" until I'm more confident of what I'm seeing and how to address the problem.
cc: @biljanaorescanin @gmao-jkolassa

@sdrabenh
Copy link
Collaborator Author

sdrabenh commented Apr 6, 2023

@sdrabenh, I'm afraid there's an issue with CatchCN in your branch. After a very quick look, here's what I think happened. I might be wrong, but what I'm seeing is concerning enough to throw a flag. In a nutshell, #637 was on a rather old branch of the GCM GC that still had only one GC F90 file for CatchCNCLM40 and CatchCNCLM45. At some time between then and now, CatchCN was restructured, and lot of the code in the CatchCN GC file moved into subdirs and GC F90 files specific to CatchCNCLM40 and CatchCNCLM45. The problem is that when you went from your branch feature/rreichle/r21c_updates (associated with #637) to feature/sdrabenh/r21c_G2G (associated with the present PR), the CN-related changes from #637 got lost. We'll need to take a closer look and, if I'm not mistaken, figure out the best way to resurrect the changes of #637 that fell through the cracks. For now, I'm holding off on hitting "approve" until I'm more confident of what I'm seeing and how to address the problem. cc: @biljanaorescanin @gmao-jkolassa

I believe that is exactly what happened. The joys of applying commits after refactoring code! I'm afraid ensuring all the changes in #637 make it into this branch will need to be done by hand. I think all automated tools will fail at this.

@gmao-rreichle
Copy link
Contributor

I believe that is exactly what happened. The joys of applying commits after refactoring code! I'm afraid ensuring all the changes in #637 make it into this branch will need to be done by hand. I think all automated tools will fail at this.

Thanks, @sdrabenh, for confirming my suspicion. Is the target branch release/v1 meant for R21C? If so, we could conceivably forget about the CatchCN updates because presumably R21C won't need CatchCN. The real question is how the R21C History updates will find their way into the develop branch. That's where we need to make sure we got everything, incl the missing CatchCN updates. Are there plans to capture the R21C History changes in develop?

cc: @biljanaorescanin @gmao-jkolassa

@mathomp4
Copy link
Member

mathomp4 commented Apr 6, 2023

Thanks, @sdrabenh, for confirming my suspicion. Is the target branch release/v1 meant for R21C? If so, we could conceivably forget about the CatchCN updates because presumably R21C won't need CatchCN. The real question is how the R21C History updates will find their way into the develop branch. That's where we need to make sure we got everything, incl the missing CatchCN updates. Are there plans to capture the R21C History changes in develop?

cc: @biljanaorescanin @gmao-jkolassa

@gmao-rreichle I can partially answer this. release/v1 is a branch created from the last v1 tag of GEOSgcm_GridComp (i.e., before Bill's new physics). So, if there are any needed patches for a GEOS model using v1.17.4, they would go here (and if still applying to current develop on a new v2 tag).

Since R21C is not moving to v2 physics, changes go there.

@sdrabenh sdrabenh dismissed stale reviews from zhaobin74 and sanAkel via 64ca333 April 12, 2023 21:44
@sdrabenh
Copy link
Collaborator Author

@gmao-rreichle My latest commit attempts to incorporate all the LONG_NAME changes into the CatchCN components so the changes do not get left behind, even if they are not used in R21C. Please review if these look ok now

sanAkel
sanAkel previously approved these changes Apr 12, 2023
Copy link
Contributor

@sanAkel sanAkel left a comment

Choose a reason for hiding this comment

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

We should really have an automatic code generator for such changes.

Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

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

@sdrabenh: Many thanks again for fixing up the CatchCN40 and CatchCN45 GridComp files. I added one commit 0179922 that updates the runoff longname, which had somehow fallen through the cracks. Looks good to me know.

@sdrabenh sdrabenh merged commit 7889361 into release/v1 Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff The changes in this pull request have verified to be zero-diff with the target branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants