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

Cmip2 #193

Closed
wants to merge 42 commits into from
Closed

Cmip2 #193

wants to merge 42 commits into from

Conversation

dabail10
Copy link
Contributor

This PR includes the following. Note these rely on the CMIP updates from Icepack as well.

Update all of the CMIP history variables #93

Update evaporation over snow and bare ice. #97

Also, some uninitialized variables. #63

Adds gravit to ice_dyn_shared.F90 when #coupled is activated. #159

There is a new history namelist flag f_CMIP that activates all of the CMIP history in one go.

Developer(s): D. Bailey

Please suggest code Pull Request reviewers in the column at right.

Are the code changes bit for bit, different at roundoff level, or more substantial? BFB

Is the documentation being updated with this PR? (Y/N) N
If not, does the documentation need to be updated separately at a later time? (Y/N) Y
Note: "Documentation" includes information on the wiki and .rst files in doc/source/,
which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.

Other Relevant Details:

@dabail10 dabail10 requested a review from apcraig September 24, 2018 21:31
@apcraig
Copy link
Contributor

apcraig commented Sep 24, 2018

This looks the same as the cmip branch. @dabail10, can you look thru the code carefully and make sure things are what you expect. I almost wish you would have left the istat calls on the cmip branch. That was a really obvious mod to check different git processes for differences in PRs.

There are some things that seem either reverts from the dummy2 branch (which we don't want) or something different than the CMIP mods. The changes in ice_dyn_shared, ice_step_mod lines 80-83, ice_grid, and CICE_RunMod all seem somewhat unrelated to the CMIP changes. So, are they mods that fix other things? Are they mods that shouldn't be there? Should there be equivalent mods in other CICE_RunMod files other than cice (ie. cesm)?

@dabail10
Copy link
Contributor Author

I thought I had detailed everything above in the comments. There some initialization issues and other relatively small changes that I figured I could combine with the CMIP changes. If this is not ok, I can try to add them one at a time. The issue is with all the cumulative changes and PRs sitting around, I just kept dealing with a few of the minor issues.

@apcraig
Copy link
Contributor

apcraig commented Sep 24, 2018

That's all fine, I'm not saying they shouldn't be combined. I just want for you to look over the code changes and make sure you don't see anything "extra" or notice anything "missing". I don't have a good idea whether this is exactly what should be in this PR or whether we still have some dummy2 merge issues or something else. I am not convinced we've found a robust process to deal with code like this. That's what I would like to figure out if possible. I don't like thinking we have to manually check all code and fix missing/extra merge stuff manually. The first step is to decide whether this PR is correct as best as you can tell. If not, then we can discuss next steps.

@dabail10
Copy link
Contributor Author

This was the same as the CMIP PR.

@dabail10 dabail10 closed this Sep 26, 2018
@dabail10 dabail10 deleted the cmip2 branch September 27, 2018 15:41
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