Refactor icepack interfaces #285
Conversation
|
Would love some feedback whether this is heading in the correct direction. I will be starting to generate some documentation on the interfaces and in particular, the tracer indexing. That will be added to the icepack user guide. I have several open issues and questions.
|
There was a problem hiding this comment.
Some of the tracers are required, and so I think they should not be initialized to 0. E.g. Tsfc is assumed to be the first tracer, so nt_Tsfc = 1. Likewise qice and qsno are required. Aren't there also indices for ice and snow volume? They don't seem to be listed in icepack_tracers.F90...
This is definitely heading in the right direction. I looked through all the code changes at this point, and entered the only thing I noticed that might need to be looked at again in the review above. I'll have to look at the code more closely to understand the details, in order to answer your specific questions, but here are some initial thoughts.
Can a slice be sent in, so that the indexing order doesn't matter? Or is the issue that more than one bgc tracer are used in the shortwave and sent together in one (bgc) array?
The 'bio grid' includes two extra grid points, to handle boundary conditions at the top and bottom, and so entails some special indexing. This is awkward but is documented. I don't think the extra bio_index arrays are absolutely necessary, but they are convenient. Most likely, what happened is that there are too many bgc tracers to send slices for each through the interface, individually, and so the whole tracer array was sent and the bgc tracers copied out of it when needed. But the whole tracer array is too much and so a subset was indexed...
My guess is it was just convenience at the time.
I think the answer is yes, we do want to fix this. The question is how and when. Would a reasonable compromise be to send the bgc tracers through, as a group? That would entail some bio indexing, but maybe it would be clearer?
Something like this would be nice, but I tend to agree that it may be more than we can do right now. Isn't the tracer get/set stuff sort of like a datatype? We'd still have to query it to get what we need?
I can explain this, and in fact this might be useful for the tutorial. I already have some slides that explain how the tracers work (we have depend/strata etc because we have tracers that are tracers on other tracers rather than on the bulk ice or snow). I suspect that some of the extra indexing can be unified but maybe not completely eliminated.
It makes sense to me to let Icepack own those too, since they are so fundamental to Icepack's parameterizations. I think the original argument for passing them in was that they were set by the user (cpp or namelist), and Icepack doesn't have I/O. |
|
Thanks @eclare108213. Regarding defaults set in the declaration of icepack_tracers. Currently, things are undefined which I think is a little worse. As currently implemented, the driver has to initialize all values, so by adding these defaults, at least we make things a little more predictable if a user forgets to set some. If there are requirements like nt_Tsfc needs to be 1, then maybe we need to set that as the default and then NOT let users reset it. As it stands, users have to know to set it to 1. My hope is that it's just required, but not necessarily as the first tracer. It would be good to add some checks on the required tracers, to check that different tracers aren't given the same index, and so forth. Regarding the tracers depend/strata stuff, my sense is that this is something that is based on the implementation of icepack. It's icepack that is defining these dependencies yet the user has to understand them and then initialize the tracer depend/strata arrays. Do I have that right? This seems a little backwards. I am now thinking we need a new interface. Something like
where names is a 1d array of character strings like ('Tsfc','qice','qsno',bgc_N1','bgc_N2',...) where those strings are limited to tracer names the code understands and would be documented. numbers is also a 1d array of the same size that defines each tracers number of tracers. Separately, we still need users to define the n_ parameters like n_algae (or we could have users define the n_ parameters first and not pass the numbers array if that could work). Anyway,from this new interface, the nt_ and nlt_ indexes can be set as well as the tracer_depend/strata arrays. Icepack should be the one defining those arrays as those may change over time as new tracers or interactions within Icepack change. It's dicey to have users have to define those tracer depend/strata arrays manually with little other checking after that. We could continue to support the current method, but try to migrate to the new implementation which provides better and more robust use. Separately, I will try to continue to look at the bio_index arrays to see if I can understand them as well as possibly propose some refactoring. Are bgc and bio the same thing? If so, I would like to move toward a uniform name if we can. If bgc is everything and bio is a subset, that's fine too. I guess I just need clarification. |
|
Just a quick comment. I have confirmed that if I reset nblyr to 7 inside Icepack, but just pass in the current slices, I recover the results for the bgcsklNICE case as we had previously bit-for-bit. That means the current implementation was getting the wrong results for bgcsklNICE due to incorrect nblyr setting in the cpps. So that answer change is correct. |
|
I've not look in details at the code changes (I don't know this area of the code, though I can take a look). I have more of a question : |
|
Tsfc used to be required for the thermodynamics, but we've added an option that allows the code to run without thermodynamics, and so in that case it might not be required. I always had it as the first tracer, and I don't know whether that is a requirement any more. My guess is that it is not, because we use the nt_Tsfc index. Could try a run with it somewhere in the middle... The idea with the depend/strata arrays is that users define which tracers they want, and then the code sets them up. I agree, this could be done within Icepack with set/get types of functionality. Part of the reason for having it in the host model is that there are different approaches, e.g. age can be a function of only the concentration, or it could be a function of the full vertical column i.e. volume, if someone wanted to track new ice growth in the column. This option isn't currently supported in CICE or Icepack (the code to do it is commented out, I think), and if someone wanted to do it, it could be defined as a new tracer. Anyhow, I (almost) always know the background "why"... and we don't have to keep things the way they are. Letting Icepack own all tracer properties makes sense to me. BGC and bio are similar, but I consider bio a subset of BGC. E.g. BGC includes dust and black carbon and other things that aren't necessarily biological (but might be biologically relevant). To me, algae, DMS, etc are "bio" but aerosols in general are BGC. I'd prefer to label them all as BGC. I think "bio" is @njeffery's shorthand for biogeochemistry, but she can weigh in with her definition. Thanks for the bgcsklNICE bug fix, @apcraig. @phil-blain, the Icepack driver and CICE driver are not entirely up to date with each other. We try, but this is a definite downside to having two separate repositories with their own drivers and testing infrastructure. I can see how this could be confusing in the documentation, so we should probably make an issue to flag it for cleanup. |
|
Just to clarify about the depend/strata situation. Those arrays are set in the driver for use in Icepack, correct? But the way tracers depend on each other is based on the Icepack implementation (any maybe options turned on within)? |
|
That's mostly right, but the transport (incremental remapping IR) also uses the depend/strata information, and in fact, the tracer structure was designed for IR and we just use it in Icepack parameterizations. There are different dependencies based on namelist inputs. Icepack parameterizations provide the physical reasoning for the dependencies; the transport is agnostic about that. So while IR is the motivation for the tracer structure (and the reason it's still set in the cice driver), perhaps those definitions do belong in Icepack and would be 'safer' there. |
rename and add 3 mushy routines to icepack public interfaces remove n_aero as passing argument from therm1 and therm2
There was a problem hiding this comment.
Some comments on variable names or their references:
nbtrcr_in -> n_aero_in -- I think this is okay as long as aerosols are the only thing being passed. However, the 'b' sometimes refers to the bio vertical grid, as opposed to the physical grid. To make things even more complicated, there are 2 aerosol schemes and the original one (from NCAR) just had aerosols on the delta-Eddington radiation grid (2 snow layers, 2 ice layers). So if the aero usage here isn't that general, maybe the b should stay. Or use z. I'm not saying that what you've done is wrong, just to be careful with aero stuff. And yes this complexity drives me a little crazy.
! algal C is not yet distinct from algal N -> ! bgc C is not yet distinct from bgc N -- change the comment back. (line 644 in icepack_tracers.F90)
Should doc/source/user_guide/lg_interfaces.rst instead be ug_interfaces.rst? Oh, I see, lg refers to library.
There was a problem hiding this comment.
Thanks @apcraig
You've done a huge amount of work on this. Looking through all the changes, only a couple of things in icepack_tracers.F90 concern me.
line 486: nfsd_in refers to categories, not layers (comment)
Categories and layers (ncat, nilyr, nslyr, nfsd, etc) have traditionally been considered 'domain size', but the domain_size module is part of the driver, not Icepack. It seems like those would be more appropriately placed in icepack_tracer_sizes. What is your reasoning for putting them in icepack_tracer_indices?
|
and my apologies for not noticing that earlier |
DO NOT MERGE THIS
PR checklist
Update Icepack Interfaces
apcraig
ENTER INFORMATION HERE
except fix for icepack bgcskl, nblyr was set incorrectly
For now, this serves as a new place to discuss code changes.
This implementation addresses
A couple of bgc cpp sizes had to be corrected for this to work. I believe the size of NBGCLYR in bgcsklNICE was incorrect and impacted the results.
See also CICE-Consortium/CICE#379
See prior discussion at CICE-Consortium/CICE#378