-
Notifications
You must be signed in to change notification settings - Fork 45
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
Katetc/dglc negfluxes #303
Conversation
… the entire ice sheet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly looks great! It will be really good to have these corrections in dglc - thank you!
The main thing I want to talk more with you about is the use of areas, as we've been discussing by email. Other than that, I just have two small comments below.
dglc/dglc_datamode_noevolve_mod.F90
Outdated
call ESMF_MeshGet(model_meshes(ns), elementdistGrid=distGrid, rc=rc) | ||
if (ChkErr(rc,__LINE__,u_FILE_u)) return | ||
call ESMF_DistGridGet(distGrid, localDe=0, elementCount=lsize, rc=rc) | ||
if (ChkErr(rc,__LINE__,u_FILE_u)) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you repeat this fetching of lsize rather than using the original lsize = size(Fgrg_rofi(ns)%ptr)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @billsacks - I think that lsize should be set as a module variable on initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just not clear on everything that is done in these two function calls, but I removed the second one and replaced it with the size(Fgrg_rofi(ns)%ptr) function instead as that does seem clear and simple. I disagree with Mariana that lsize should be a module variable, as I think that would require writing it out for restarts, and that's unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KateC - I got rid of lsize as a module variable in my PR for the restart problem - so we're on the same page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just not clear on everything that is done in these two function calls
The ESMF_MeshGet call says: I've got this mesh (model_meshes(ns)
), give me the associated elementdistGrid (distGrid
); then ESMF_DistGridGet says: I've got this distgrid, give me the elementCount (lsize
). (This is a common ESMF pattern: I've got this object, I want to get one or more of its components or other related information, so call a Get
function that takes that object as an argument and various optional output arguments.)
@Katetc - it's important to not change the order of the initialize_noevolve flag in dglc_datamode_noevolve.
This order needs to be kept since Flgl_qice(ns)%ptr(ng) is not actually received the first time this is called from InitializeRealize in glc_comp_nuopc.F90. |
…-0 case is handled correctly
@Katetc @billsacks @jedwards4b - since this PR changes answers I think it should be a separate PR from the fix to the restart for multi-level ice sheets. Also - this is still under development and NorESM really needs the fix for the restart as soon as possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you have addressed the earlier comments - thank you!
Note that I did run the aux_cdeps test suite on Derecho with the intel compiler and several tests failed at create_newcase due to the removal of grid aliases in ccs_config_cesm PR#177 ESMCI/ccs_config_cesm#177 . I will add an issue to update the test list. |
@KateC I will update the tests in my incoming rpointer timestamp PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two very minor remaining points - a line of code that I think you can remove, and a stylistic nitpick that I'm okay with you ignoring. Otherwise, looks great! Thanks again for your work on this!
Thanks everybody for the reviews, comments and help! I have completed some one-off conservation checks and looked at my results, and they all look good. I'm doing one more quick test right now to make sure that my final modifications didn't break anything unexpectedly, and then I feel this will be ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now - thanks for the last couple of changes!
Let us know when you feel this is ready to merge.
Ok, I have run my tests and everything looks very good to me this morning. I just tried to automatically merge in Jim's morning PR, so I guess we'll see how that goes. Once it's done, this one is ready to go next. |
Description of changes
Add code to Dglc to balance neg and positive ice fluxes and reduce the amount of negative water fluxes coming from each ice sheet to the ocean.
Specific notes
Contributors other than yourself, if any:
CDEPS Issues Fixed (include github issue #): #301
Are there dependencies on other component PRs (if so list):
Are changes expected to change answers (bfb, different to roundoff, more substantial):
Any User Interface Changes (namelist or namelist defaults changes):
Testing performed (e.g. aux_cdeps, CESM prealpha, etc):
Hashes used for testing: