update icepack calls in CICE to add keywords #378
Conversation
apcraig
requested review from
eclare108213,
mattdturner,
dabail10,
proteanplanet,
duvivier and
phil-blain
There was a problem hiding this comment.
Without looking super closely, this line looks confusing to me, since trcrn is a standard array that is much broader than the bio tracers (which is what I think ztrcr is):
(line 353 in ice_init_column.F90, line 968 in ice_step_mod.F90)
trcrn=ztrcr,
Also, it looks like Icepack is changing too. Is that intentional?
|
@eclare108213 here is what I found: CICE/cicecore/cicedynB/general/ice_step_mod.F90 Lines 889 to 891 in d3d1aaf It is initialized at the beginning of the i,j loop in this subroutine: do j = jlo, jhi
do i = ilo, ihi
! ...
ztrcr_sw(:,:) = c0
do n = 1, ncat
do k = 1, ntrcr
ztrcr(k,n) = trcrn(i,j,k,n,iblk)
enddo
if (tr_brine) fbri(n) = trcrn(i,j,nt_fbri,n,iblk)
enddo
if (tmask(i,j,iblk)) then
call icepack_step_radiation (dt=dt, ncat=ncat, &
! ...
trcrn=ztrcr, &So from what I understand it is used to pass the contents of the tracers array at indices (i,j,iblk) to However in the call to "trcrn" refers to the name of the dummy argument in the subroutine definition in Icepack: [I think there is no preview of the lines here because these lines are in Icepack and this PR is in CICE. -1 for GitHub.] So I agree it can be confusing but the only thing we could do to avoid the confusion is change the dummy argument name inside Icepack... |
There was a problem hiding this comment.
I tried to check every change thoroughly, and I think everything is in order.
A couple comments:
- Is there any reason you changed the order of arguments at a few places ?
- Most arguments lists spanning several lines in CICE are aligned with the opening parenthesis (eg. calls to
icepack_ice_strength), but the calls toicepack_step_radiation,icepack_init_zsalinity,icepack_init_OceanConcArray,icepack_init_bgc,icepack_init_hbrineare not aligned.
This is not major but I think consistency in this regard is important for the quality of the code, and how easy it is to read and skim it.
Coding style aspects like (2.) are not mentioned anywhere in the dev wiki pages, but I think it could be good to have a "coding standard" document for CICE and Icepack, or link to an existing one. We could talk about this this afternoon maybe.
|
Also some tests are failing (mostly with the PGI compiler I think), do we know why ? |
|
I like what you've done here. I don't have any additional comments beyond what Phil and Elizabeth have already raised. |
|
Good catch about the argument name (trcrn) in icepack_step_radiation. I meant to make a note of that when I made the changes. I think we should change the name of the trcrn dummy argument in icepack_step_radiation and then update the interface in this PR. As I added the keywords, this one really stood out as being less than ideal. This is the time to make this change before we are readily using the keywords. Are there any other icepack interface arguments we would like to rename? I think we need to do the following. 1) Identify the icepack argument names we want to change. 2) update the icepack interfaces and create an icepack PR. 3) Update this CICE PR with the new icepack version and update the icepack calls in CICE. If folks can help with 1), I can take care of the rest. Regarding the pgi failures. This is consistent with master. Those failures have been persistent on izumi, and it's something we could look into a bit more. I tend to have little faith in pgi so when 100% of other compilers are passing and we're getting >90% on pgi, I think that's pretty good. Regarding the coding style of the interface calls. What I tried to do is make each interface call readable following a couple of styles that mixed the indentation, aligning arguments, aligning equal signs, having one or more than one argument per line, line lengths, and spacing. I fully recognize the indentation/style is not consistent between them, but that's generally the case throughout the code. I'm not 100% convinced we need complete uniformity in this area. I think the goal should be some level of readability and consistency with current standards, and I think we're doing that. I'm happy to make changes to the current updates if that seems helpful. Also if we want to come up with a formal standard that is enforced, I'm willing to go thru the code and update all interface calls (not just icepack) to meet that standard. That's another way we could go, but I'm not sure uniformity across interface calls necessarily equates in all cases to improved readability. Regarding the argument order. I did make some minor changes in the order of arguments in the icepack calls in CICE. This was done mostly (I hope) to improve readability. These changes are pretty minimal. As we move to keywords, I think one of the benefits is not having to match up argument by argument with the interface in the same order, and I think we should encourage the community to take advantage of that moving forward. |
|
I'll try to read through the Icepack interface argument lists and flag less-than-ideal names, but it'll be next week before I have a chance, so it would be great if someone else wants to do it. On coding style: I also noticed this but did not flag it. We do not have consistency across the code now, although I have tried to move toward better consistency as changes have gone in. @apcraig captured the various options that we've used in the past, in his comment. My preference is to keep things aligned the way they are when there are substantial changes, so that code diffs show the important stuff. That said, this is a good opportunity to at least make these argument lists more consistent in the code. I'd be open to a massive cleanup/realignment, but it's not a high priority in my opinion. The last time a big cleanup was done was probably CICE v5, maybe v4, and that wasn't completely consistent either. Readability is the main goal. Argument order: Does changing the argument order in CICE affect existing Icepack implementations in other host models? |
|
I will have a look and try to point out any odd looking argument names. There really are just a handful of arguments in the current implementation where the keyword and value are not the same in the CICE calls. I'm not convinced that means the dummy arguments in Icepack are best, but at least it indicates some consistency as the code has evolved. I recently started a section in the icepack documentation on "use of icepack in other models". I think we need to formally document each public interface in the Icepack user guide, and I think we need to provide at least an outline of the calling sequence and some description of how to use it. I am thinking about generating a tool that quickly extracts the public interfaces from the icepack code, so we can drop that into the document. It would probably involve adding some simple keywords like "!public_interface_doc_start" and "!public_interface_doc_end" to trigger the extraction. That is not ideal but better than nothing, and we have to make sure we keep the document up to date. One good thing is that our current interfaces are pretty well self-documented. I think we should add a paragraph in each public interface that briefly describes what it does. An alternative would be to use something like doxygen which I'm pretty familiar with. The benefit there is we might automatically generate that thru github and it would stay up to date automatically (hopefully like we do now with readthedocs). The negative is that we'd have to make some efforts to add doxygen syntax for parsing and then we'd want to keep that up to date. One good thing is that syntax is pretty nonintrusive. Maybe I'll add an issue for this in icepack. On coding style, as I said, I'm open. If someone wants to define a standard, I'm willing to help us move that direction. |
|
I think something like Doxygen would be very beneficial, for both CICE (mostly for us developers) and Icepack. It could directly be incorporated into our read-the-docs documentation, in a separate section (API or something similar.) Another more modern option is FORD : |
|
It would necessitate some work upfront to convert the code to use the right syntax for declaring procedure arguments and their description, etc. but in the long run the API doc updates itself as we update the code so that's very appealing. |
|
I have created an issue in Icepack to discuss documentation. Lets take that part of this discussion there. CICE-Consortium/Icepack#283. Thanks. |
|
For those that want to weigh in on changes to the icepack dummy arguments, there is a google doc that condenses all those interfaces to one place, https://docs.google.com/document/d/165sAt7Y9sipRvNTyB8oeOCpgFt_rHY0_3RsSvSuTe78 I have been using that to help me understand the arguments, intents, and optional settings. It would be an easy place to have a quick look. We could completely revamp the argument names if we wanted. They are currently based on the internal variable names used in CICE which is why the keyword and arguments match so closely. I'm not suggesting we do that, but we could. It would be a lot of work to do that because we'd probably want to change that variable name throughout Icepack, but if we want to go that route, now is the time. Personally, I think we should just find any egregious argument names and change them. An important point is once we settle on something, we'll want to try to stay with it forever due to backwards compatibility. If we are encouraging users to move to keywords in interface calls, we should not change those keywords once established unless there is a really good reason. Now is the time to review this. My list of arguments to think of changing are A few comments. As a general rule, I think we should unify the names for the 2d trcrn(ncat,ntrcrn) and the 1d trcr(ncat). In the same way aicen,vicen,vsnon are size (ncat) while aice, vice, vsno are scalars in all interfaces. The argument trcrn in icepack_step_radiation seems fine to me. We are actually passing a copy of all tracers into icepack. The real issue seems to be whether ztrcr and ztrcr_sw are reasonable names for temporary arrays in ice_init_column and ice_step_mod. Personally, I think these should be renamed trcrn_tmp and trcrn_sw_tmp or something like that in those subroutines. The main thing is that the zbion argument should be changed to trcrn_sw in my opinion. I think the max_ names have a unique purpose in the code that isn't consistent with them being dummy arguments to an interface. |
|
@phil-blain thank you for investigating the ztrcr issue. I dove a little deeper and found that this array is used for calculating the feedback of bio tracers on the radiation balance. (Need to look further to make sure that's the only use.) When we first started separating the column physics from CICE to make Icepack, we decided to only pass the slices of trcrn that were needed, rather than the whole array, the argument being that other models (e.g. MPAS) would not want to pack and unpack their tracers into a big trcrn array for every Icepack interface call. Sending slices is expensive, I know, but it's more flexible for users; we have trcrn in CICE mainly for convenience, driven by how the incremental remapping advection in the dynamical core works. So, I have tried to be vigilant in sticking to our 'rules' for these interfaces, but this one got by me. The right way to fix it would be to just send the slices that are necessary, which would be another non-backward compatible change. |
|
@eclare108213, OK, that makes sense. I think now is a reasonable time to change the trcrn argument in ice_step_radiation and changing that to a subset of tracers (slices). We are adding other non-backwards-compatible changes with fsd. I can help implement or someone else can. What we need to define is the name of that argument as well as what slices need to be passed from CICE. We then need to check the underlying implementation in icepack to make sure the array indexing is consistent with the changes in the argument. Separately, does something similar need to happen with the zbion argument (which I propose to rename trcrn_sw). |
|
I'll look through the interface names and dig a little deeper into the code for the ones that might need to be changed, this afternoon. |
|
Here’s my take on this. First, let’s not revamp all the argument names. They are workable now, although some could be chosen better and there are a few that are being used inconsistently. Regarding the naming convention, e.g. aice and aicen, trcr and trcrn: The n refers to thickness categories. aice is a scalar but can mean 2 things: aggregated (averaged) over all categories, or a single value from one category, which is sometimes (confusingly) aicen. For single-category values, we used ai in early thermo codes, but the trend is toward longer, more understandable variable names, so we have a mix now. I've not worried about it too much because it's usually clear in the code that it's a scalar, and only very rarely (if ever) is it the aggregated value. trcr and trcrn are a little more difficult because they are often arrays, even when aggregated or being used within a single category. The code change that removed the sizes from the array declarations made this much more opaque. Again, I'm not too worried about this within Icepack. My recommendation is to make the interface naming self-consistent on the driver side, and then worry about making Icepack itself consistent later, more gradually. Despite what I said about sending slices, there are several Icepack interface calls that require the entire tracer array, because changes within the column affect all tracers, agnostically. Examples are the ridging, fracturing by waves (in FSD), some parts of step_therm2, and aggregation over categories. Often the clue is that trcr_depend, trcr_base, n_trcr_strata, nt_strata are also passed.
I wouldn't change this one unless you're going to change all of the others (atrcrn, aicen, etc), which would be fine. The routine is at least internally consistent.
I disagree. See comments below.
Agree, but I'd use trcrn_bgc instead of bio, because there's chemistry too
These should probably be changed to n_algae etc and allocated. I’m not sure why we didn’t do that before, except that the bgc code is a bit of a beast… It seems like these arguments shouldn’t be necessary at all, if icepack already knows about icepack_max_*, but if we do want to allocate them at some point, then the namelist values will need to be passed in, at least for the initialization. In icepack_init_ocean_conc, they are only used as loop limits that set arrays to constant values, so they don't need to be sent through that one at all.
If the array has an ncat argument, then I think it should be arrayn. Do we have examples of trcr(ncat), that aren't renamed according to the specific tracer that's being sent through (i.e. the slice)? If I understand your logic above, I think you mean "1d trcr(ntrcrn)"?
But we shouldn't be. What I truly do not like is that the BGC code loads the entire tracer array and only uses parts of it, and also creates new arrays that look and feel like the full tracer array (e.g. trcrn_sw) but only contain bgc tracers.
‘z’ indicates they are part of the vertical biogeochemistry code, which is meaningful. The problem is that the entire trcrn is being sent into the subroutine, and ztrcr_sw is being initialized to zero for the total number of tracers, but only a few bgc tracers are actually used. Instead of sending the appropriate slices and naming them appropriately in Icepack, trcrn and ntrcr are used as dummy variables and only parts of them are accessed. This needs to be cleaned up — in fact a lot of the BGC code needs to be cleaned up. This is where my punting on BGC when we were fixing the interfaces the first time comes back to bite us. It’s a mess. I don’t know how difficult it would be to just send the required slices through the interface, but that would be good to do now rather than later. Can we figure what minimally would have to be done to keep the interfaces backward-compatible?
Agreed. However I think trcrn_bgcsw might be an even better name, since the tracers in question are aerosols and chlorophyll a, i.e. chemistry AND biology, and no others. Or, if sending a slice instead of the whole tracer array, call it zbgcn. That (or zbion) makes more sense deeper in Icepack.
Agreed. They're left over from pre-Icepack-interface days. |
|
I'm fine with all that. The one that needs a bit of work is icepack_step_radiation with trcrn and zbion. Do you have an idea what slices need to be carved off and if we can pass is a single subarray of trcrn and trcrn_sw or if we need to add new arguments to support multiple multi-slice sections? If you could define the new arguments on the cice side, I'm willing to go into the icepack side and fix the references and calls further down. I don't know how many layers this will affect, but if it's the right thing to do, we should just do it. |
|
It looks like the radiation only uses these tracer indices: 'nlt' means the index is in the special bgc tracer numbering system; the others are standard trcrn indices. sigh |
|
I would like us to also review the interface names My feeling is the naming is too similar or too different. If they are closely related, then we should name them ocean_conc_array and ocean_conc or similar. If they are not that closely related, then we should name the ocean_array and ocean_conc or something else. It just seems to me the names we have now are not ideal. |
|
I looked at ice_step_radiation a bit more and agree with @eclare108213 that there really are only are only a few indices used from the trcrn and zbion arrays. But the subarrays used are also multi-dimensional. From my assessment, trcrn references elements zbion references elements but It's not clear to me that we can/should assume that those arrays will always be ordered in a certain way in the driver. The other odd thing is that the nlt_ index values are passed in (but also known in icepack via a use statement) while the nt_ index values are just from the internal use statement in icepack. So that is also confusing. I agree with @eclare108213 that we should be sending appropriate subsections of arrays (aka slices). We need to think about this from the icepack side. Icepack needs to specify what's needed and how it's passed. And we need to make that general-ish. Even looking at the array hpndn (to take an example), that is a one dimensional array. That argument should be documented as hpndn of size ncat and some standard for the order of the array from lowest to highest or whatever. Much of this is currently taken from the CICE implementation and there is some sort of implicit understanding of the Icepack arguments and what they mean, but if we really want Icepack to be a usable library for other models, these interfaces need to be well thought out and well documented. I know I keep saying that, but I think it's incredibly important. I actually don't know the underlying implementation well enough to refactor all of this or even make particular recommendations. That's always been a shortcoming for me. Mostly, we're in reasonable shape. But a few interfaces seem pretty messed up. We need to define from the Icepack side, the different arguments and the array ordering and sizes. I think we also need to make the indexing consistent. Are we setting the indices in icepack and having icepack use those internally defined values? Or are we passing index sizes into the interfaces? We should have some consistency. I will create a sandbox and see if I can sort some of this out. But I'm happy to have input. |
These arrays are on the 'bio grid' which is a vertical grid with 2 extra layers for boundary conditions. The n argument captures different species, etc.
similarly, these are on the physical vertical grid... I think. Is that a bug in nlt_chl_sw -- should it be nslyr + nilyr?
We could clearly document what the assumptions are.
I recommend cleaning this up, only passing in/out things that are needed in the CICE driver through the interfaces, and 'use'ing anything else (or whatever makes sense within the Icepack context). For BGC, the current implementation might actually make sense (see below), though I'd want to double check.
I think the key here is to document what we have. In the course of that, we may see things that are not consistent or well thought out, or could be done better. I understand the reasoning behind what's been done for the various tracers, including BGC, so can help sort out the underlying implementation, if it gets too murky.
The index sizes need to be passed in when they are defined via namelist. That's particularly true for BGC tracers, for which we can configure them in various combinations. Other tracers are fixed by the particular parameterization, and the namelist only turns the entire set of pertinent tracers on and off. This is partly what makes the BGC code so squirrelly. Again, documentation is key, and I believe @njeffery provided enough info in the Icepack docs to figure it out (while looking at the code). We need to decide whether we want to provide detailed documentation for each tracer, individually, or for classes of them. I might favor the latter. Another option could be to only have a finite set of possible BGC tracer combinations, which would be selected via namelist without setting individual BGC tracers. Then we could provide some documentation on how to create other BGC configurations, for experts. I'd like @njeffery to weigh in on whether that's really feasible and if so, what the finite set of BGC tracer combinations would be.
Let me know if I can help... |
These are related. icepack_init_ocean_conc initializes the concentration values for various BGC tracers, and the icepack_init_OceanConcArray loads them into a big array, ocean_bio_all. We could rename them to indicate that it's bio related (I always think "what's 'ocean concentration' mean?" when I see this, as if it's analogous to ice concentration) and their more specific purpose. How about icepack_init_ocean_bio |
|
@MichaelWinton @lzampier @proteanplanet and others who use Icepack within host models other than CICE: I want to bring this PR to your attention. There are a few non-backward-compatible changes to the Icepack interfaces being considered here, in preparation for the next release. The goal is to fix the interfaces so that backward compatibility is easier in the future, so that users do not have to keep changing their host models' interface calls when upgrading to newer versions of Icepack unless they want to take advantage of new capabilities (e.g. the floe size distribution in CICE-Consortium/Icepack#281 will add new arguments and change at least one of the existing ones). We'd appreciate your input on the changes here -- whether they are helpful or not, if they address any significant issues you encountered, what major headaches we aren't thinking of, etc. The main change is to encourage use of keywords in the host model and (eventually) make (some) arguments on the icepack side be optional, but we've also noticed a few inconsistencies that we'd like to fix, as long as we are making non-backward-compatible changes anyhow. The heart of the conversation begins here: #378 (comment) and of course you can look at the proposed code changes directly. There is also an Icepack PR to improve the documentation for the interfaces: CICE-Consortium/Icepack#284 (discussion is in CICE-Consortium/Icepack#283). We'd appreciate whatever comments/suggestions you might have. |
|
I like the new oceanconc interface names better. We can add that to the list. Regarding the tracers arrays and indices. I also agree we need to come up with something consistent. At the present time, we seem to be mixing passing in a multi-tracer array vs sending slices (even if multi-dimensional by category or some sort of vertical dimension). We are also mixing passing index values vs using the ones set inside Icepack. I believe we have "init_tracer" interfaces that can set all of the indices if that's what we want, even the nlt tracers. It seems we are tending toward using tracer slices. But we may also need to send a full tracer array (with appropriate trcr_depend+3 arguments). It would be nice if the interfaces that "require" the full tracer array could be called on multiple tracer arrays (say a physics array and a bgc array or even more granular) with separate trcr_depend+3 arguments. But I don't think that's possible with some interfaces like therm2, although I need to look further to confirm. If Icepack requires that all tracers be packed into a single array, then we may want to rethink sending slices it seems cleaner. Sending in the full array all the time provides some extensibility by being able to access new tracers without having to add new arguments. On the indexing front, as I said, I think we either need to required the indices be set thru the init_tracer calls and then NOT passed later or we need to pass those indexes all the time. As we move to slices, we sort of need the indices less. We also should be checking that tracer array sizes are as expected by comparing their size to the index values. We should have more checks in general in the Icepack interfaces to make sure things passed in are as expected (mostly with respect to size). I is currently very easy to pass in a slice that is the wrong size or the wrong tracer, but for things to just merrily move along (especially if the array passed in is too big). Obviously, all these things interact with each other in the implementation. I'll try to better understand what's currently in the code so I can contribute better to the decision of how to proceed. But I would like us to have a few simple "rules" for users in this regard. |
|
Regarding the question: Should we send Icepack the full tracer array instead of slices? The original argument for slices was so that Icepack wouldn’t impose a data structure on host models. Ideally, we only send slices needed for physics calculations in a given part of the code. I think this is mostly true in the code except for BGC. However, the full array must be sent through sometimes, because some calculations are agnostic, i.e. they must be done for all tracers (e.g. when ice melts away completely, category changes due to ridging, etc). Sending the full array all the time would be more easily extensible for maintaining host model interfaces with Icepack, since we would not be adding new arguments to interface calls, only accessing new tracers when desired. But this likely means more loading/unloading of that array in host models and in Icepack. Note: We have changed the interfaces from what is currently in MPAS-seaice. Icepack is considered to “own” Icepack data/parameters, and now there are set/get interface calls in the host model for things like tracer indices, physical constants, etc. — those are not sent through argument lists in the physics (mostly). My inclination is to stick with what we've got (slices when it makes sense for the physics, full array when necessary, use set/get for all indices), but if those of you developing host models that use Icepack would prefer to just send the full array all the time and let Icepack load/unload as needed, we can do that. Now is the time -- we don't want to keep making non-backward-compatible changes to the Icepack interfaces. Opinions? @akturner @proteanplanet @MichaelWinton @lzampier |
|
Email comments from @akturner:
|
|
I generally agree with recent comments about how to pass tracers. I think one thing we have to do is explain the indexing of both slices and full tracer arrays (and their associated arguments) better via documentation. |
|
First of all, sorry for my delayed answer and thank you for involving us in the discussion! I agree with the previous comments, we would also prefer to pass to the Icepack subroutines only what is needed. I am still new to the code and this helps me to better understand what is modified in the computation and where problems arise. This strategy might require some more work on our side for major releases, but I do not necessarily see this as a problem. The array structure that we implemented in FESOM to store Icepack variables is basically identical to that of the single-column drivers and this helps to keep the code consistent.
Also in our FESOM implementation, Icepack is considered to “own” the data and parameters. |
|
I agree with Adrian - please keep things as they are. |
|
@eclare108213 and I had a discussion offline and we are going to merge these updates now. They are backwards compatible and only add keywords to the icepack calls which is something we want. This will help move the fsd work forward. We can continue to discuss next ideas and development with respect to Icepack interfaces in this PR, even though it will be closed. But I also plan to open a new PR soon with some non backwards compatible changes as we've discussed here. Basically, we are going to merge this now as it's relatively straight-forward and tackle on some next changes in a separate PR. |
PR checklist
Update icepack calls in CICE to include keywords
tcraig
#d3d1aafe588ed7 at https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks, full test suites on izumi (4 compilers) and gordon (4 compilers)
Updated the icepack calls in CICE to add keywords.
Updated icepack to the current master version.