search

ESCOMP / CISM-wrapper

Community Ice Sheet Model wrapper for CESM
http://www.cesm.ucar.edu/models/cesm2.0/land-ice/
Other
3 stars 16 forks source link

Add nuopc cap #44

Closed billsacks closed 3 years ago

billsacks commented 3 years ago

Opening this on behalf of @mvertens . I'll review it and then bring it to master.

billsacks commented 3 years ago

@jedwards4b in 0eddabc66a1c8f858d8312ddf2a4116c7c1dd11a, you reduced the default log level of CISM. Was that needed for nuopc, or did you just do it because you wanted less output? I'm reluctant to reduce the log level without checking with land ice scientists, so I'm reverting that commit, but please let me know if it's actually needed.

jedwards4b commented 3 years ago

I have an open issue here - I guess I understand if you feel the need to increase output again but somehow someday we need to address the issue of components writing to stdout.
https://github.com/ESCOMP/CISM/issues/32

billsacks commented 3 years ago

Thanks for clarifying @jedwards4b . It sounds like the right solution is to wait until someone has time to address ESCOMP/CISM#32, unless this has become an urgent issue.

jedwards4b commented 3 years ago

It's never an urgent issue - that's the problem.

jedwards4b commented 3 years ago

I also note that this is a namelist default setting and very easy for a scientist to increase if needed. The original setting of 6 is extremely verbose and provides nothing useful to anyone other than a land ice scientist and I would bet it isn't useful to them most of the time.

billsacks commented 3 years ago

I also note that this is a namelist default setting and very easy for a scientist to increase if needed. The original setting of 6 is extremely verbose and provides nothing useful to anyone other than a land ice scientist and I would bet it isn't useful to them most of the time.

I have no objection to turning it down if you check with the relevant people to get approval. I just don't want to fold this into this PR without approval.

whlipscomb commented 3 years ago

@jedwards4b - If we resolved the issue of CISM components writing to stdout, would that make it unnecessary to change the default setting? I do, in fact, often use this setting. I just submitted a paper, which means I now have some time to work on deferred CISM maintenance, in coordination with @billsacks.

whlipscomb commented 3 years ago

@billsacks - I wanted to follow up on the question I asked last week:

If we resolved the issue of CISM components writing to stdout, would that make it unnecessary to change the default setting?

I'm happy to update the write statements in Glissade. I'd just like to be clear on the required changes, and was wondering if I should fold these CISM changes into my work on multiple instances.

billsacks commented 3 years ago

@whlipscomb I'm not entirely sure what @jedwards4b wanted, but based on https://github.com/ESCOMP/CISM/issues/32, I'm thinking:

(1) As @jedwards4b said, print should be replaced with write(stdout, ). I think that, in a CESM run, this leads to messages appearing in different files depending on whether they are written by the master task or a different task.

(2) If you notice any writes that are done on all tasks but really just need to be done on master – or not at all – then changing the code appropriately.

If these changes are extensive, then it could be better to put them in a different PR, though if you prefer to put them in the same PR that's okay, but I'd suggest at least doing them in separate commits to aid reviewability of the PR.

jedwards4b commented 3 years ago

Thank you for addressing this issue. Note that item (2) of Bill's message can be accomplished by simply changing the default log level. We should really try to avoid output from all processors whenever possible.

whlipscomb commented 3 years ago

Thanks, @billsacks and @jedwards4b, I can work on this in the next couple of weeks. Besides fixing print statements, I'll try to avoid writing output from more than one processor.

mvertens commented 3 years ago

It should be removed - and it is in my updated branch.

On Tue, Nov 24, 2020 at 1:09 PM Bill Sacks notifications@github.com wrote:

@billsacks commented on this pull request.

@mvertens https://github.com/mvertens please see a couple of questions below about whether we should be doing some consistency checks

In drivers/cpl/nuopc/glc_comp_nuopc.F90 https://github.com/ESCOMP/CISM-wrapper/pull/44#discussion_r529845517:

  • ! write(6,102) abs(mesh_lons(n) - lons_vec(n))

  • !102 format('ERROR: CISM lon diff = ',f20.10,' is too large')

  • !call shr_sys_abort()

    • @mvertens https://github.com/mvertens - is there a reason that the aborts are commented out for the various checks here – on lons, lats & areas? Maybe we don't want to compare areas, but should we at least be comparing lats and lons? It seems like either we should uncomment these commented lines or stop comparing lats & lons at all.

In drivers/cpl/nuopc/glc_comp_nuopc.F90 https://github.com/ESCOMP/CISM-wrapper/pull/44#discussion_r529845906:

  • ! elemAreaArray = ESMF_ArrayCreate(DistGrid, mesh_areas, rc=rc)

  • ! if (ChkErr(rc,LINE,u_FILE_u)) return

  • ! call ESMF_MeshGet(Emesh, elemAreaArray=elemAreaArray, rc=rc)

  • ! if (ChkErr(rc,LINE,u_FILE_u)) return

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CISM-wrapper/pull/44#pullrequestreview-537862904, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4XCE3NFMZNG6N4ZLBAU33SRQHHXANCNFSM4TSQ2MZQ .

-- Mariana Vertenstein CESM Software Engineering Group Head National Center for Atmospheric Research Boulder, Colorado Office 303-497-1349 Email: mvertens@ucar.edu