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

CMIP changes #219

Merged
merged 6 commits into from
Sep 21, 2018
Merged

CMIP changes #219

merged 6 commits into from
Sep 21, 2018

Conversation

dabail10
Copy link
Contributor

@dabail10 dabail10 commented Sep 6, 2018

This PR is necessary for the CMIP changes (CICE Issue 93) and also add changes for the evaporation over snow and sea ice issue (CICE issue 97). The CICE related CMIP changes are coming in a future PR. Here is a short summary of what has changed:

  1. fcondbot and fcondbotn are added so they can be shared with the CICE code.

  2. Tbot is now shared with the driver and hence CICE.

  3. A new diagnostic variable Tsnic has been added for the snow-ice interface temperature.

  4. New variables evaps and evapi along with category versions have been added to keep track of evaporation over snow and bare ice separately.

  • 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) N
    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-icepack/.

  • Other Relevant Details:

Copy link
Contributor

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

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

I think we need a different variable name for Tsnic. I interpreted it differently: Ts surface temp, n on categories, i ice... what does the c stand for? Celcius? I guess sn is supposed to be snow. Maybe Tis_intfc? What does the CMIP6 standard want it to be called?

Otherwise these changes all look fine to me. Does the documentation need to be updated?

@eclare108213
Copy link
Contributor

DMI would like to merge their (CICE) code with dynamic allocation of variables in the next few weeks, and they'll start with CICE master. If there are a bunch of new history variables to add, we should try to do that first to avoid conflicts.

@dabail10
Copy link
Contributor Author

dabail10 commented Sep 6, 2018

I was thinking T sn(ow) ic(e). Would you like Tsnowice better? I would definitely like the CMIP-CICE changes to go in soon. I need the first pull request with the dummy stuff merged before I can do this though. The documentation will only need to be updated for CICE I believe, not icepack.

@eclare108213
Copy link
Contributor

Tsnowice sounds like we have a temperature for a snow-ice layer, which we don't resolve (yet!). How about Tinterface? I'd turn that into Tintfc...

@dabail10
Copy link
Contributor Author

dabail10 commented Sep 6, 2018

I was concerned about the use of intfc, because if the icepack_intfc.F90 module.

@eclare108213
Copy link
Contributor

Ah. Too few words to describe different things. How about Tsi_interface, then? Or Tsnice if that's not already in use (it looks kind of familiar).

@dabail10
Copy link
Contributor Author

dabail10 commented Sep 6, 2018

I did not find Tsnice anywhere, so that is good.

Dave

@dabail10
Copy link
Contributor Author

Renamed Tsnic to Tsnice.

@apcraig
Copy link
Contributor

apcraig commented Sep 21, 2018

@eclare108213, can you approve or comment then I'll start merging.

@eclare108213
Copy link
Contributor

This looks fine to me. There is a single change to the shortwave module, which is commented out. It's better not to have stuff like that in there unless there's a meaningful reason for it. But I'm okay with merging this now.

@dabail10
Copy link
Contributor Author

This changes answers, so I am waiting to commit it separately.

Dave

@apcraig
Copy link
Contributor

apcraig commented Sep 21, 2018

Just to be clear, it's that shortwave change that is commented out that changes answers, right? That's a little scary, it's just an intent change. Does is change science? I guess we can talk about it when the change is proposed later.

@dabail10
Copy link
Contributor Author

Exactly. I am unsure so far why it changes answers. It is something to do with the setting the initial snow depth and fraction related to the level ponds. These are set to zero before the call to shortwave_dEdd_set_snow, so they should be intent(inout) in this subroutine.

@apcraig
Copy link
Contributor

apcraig commented Sep 21, 2018

I just had a quick look at the code too. I agree they should be in/out. But it's odd that it changes answers and that the intent issue wasn't picked up before now. Interesting. thanks.

@dabail10
Copy link
Contributor Author

The intent inout was only picked up by uninitialized variable checking with the NAG compiler.

@apcraig apcraig merged commit bc64e87 into CICE-Consortium:master Sep 21, 2018
apcraig added a commit that referenced this pull request Sep 21, 2018
@apcraig apcraig mentioned this pull request Sep 21, 2018
apcraig added a commit that referenced this pull request Sep 21, 2018
@dabail10 dabail10 deleted the cmip branch September 27, 2018 14:54
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.

3 participants