-
Notifications
You must be signed in to change notification settings - Fork 47
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
Correction to the definition of "ocean sigma over z coordinate" in Appendix D #314
Comments
I think there is a problem here with an implicit assumption about the numbering convention for whether k=N is the surface or the bottom. In ocean_sigma_coordinate is does not matter if a user numbers from the surface sigma(1) = 0 to bottom sigma(N) = -1, or as in the ROMS model for example sigma(1) = -1 at the bottom and sigma(N) = 0 at the surface (k increasing in the same direction as z). As written, the definition above requires k=1 be sigma=0 and z=eta. The reverse order is not supported. |
Dear @johnwilkin Thanks for you comment. Yes, that's good point. I think it's a separate problem, but I agree the text should be reworded to avoid the implicit numbering convention. In fact I don't think the numbering needs to be stated at all. It could just describe the treatment of the sigma levels and depth levels without mentioning Do you agree with the proposal for what should be stored in the vertical coordinate? If I have understood the definition properly, I think it must be the case that the highest Jonathan |
We need auxiliary information for sigma(k) to make the definition generic. Something like this ... check my math and whther it's < or <= If k numbers from top to bottom then (left side of diagram attached) If k numbers bottom to top then (right side of diagram attached) With sigma defined thus as part of the standard, and/or as 1-D vector of sigma(k) saved in the output file (this is what we do in ROMS to accommodate multiple alternate stretchings in the"sigma" space for ocean_s_coordinate), and zlev included in the output as a 1-D vector, then the rule is generic. This will work for both numbering conventions: I don't know how you write this in CF-speak. But it's a robust way to code a calculation of z(n,sigma(k),i,j) |
One more point ... working from sigma(k) encoded in the file would set the scene to use ocean_s_coordinate or ocean_s_coordinate_g1 (g2) over z coordinate with minimal added hassle. |
Dear @johnwilkin Thanks. That's very helpful. Yes, I agree with you. Cheers Jonathan |
I have created pull request #317 to implement this. If @johnwilkin and @davidhassell (who originally discovered this problem) and anyone else could have a look at it, that would be most helpful. Thanks. |
@johnwilkin, thanks for your comment on the pull request, which I repeat here for the record:
|
Hello @JonathanGregory and @johnwilkin, Thanks. The PR looks good to me, but I'd like to check it with my cf-python implementation (or vice versa), and I won't be able to do that until after the weekend, I will get back to you next week. David |
Hello, In the proposed text we have: The parameter I'm still not sure how the user can know if the layers are numbered top-own or bottom-up. @johnwilkin commented: Regardless of how the modeler orders the k index it is a universal convention that sigma, or s, increases upward from -1 at the bottom (or of c_depth) to 0 at the sea surface. Does that mean we should work it out from inspection of the sigma values - decreasing sigma values indicate top-down, and vice versa? If so, it would be good to state that... Thanks, |
Dear @davidhassell Thanks for raising this point. I agree with your suggestion (when we spoke) that it would be better to require In writing this, I wondered why we assume that layers should be numbered from 1, rather than 0, or any other number. We don't have to make any such assumption, since layer number is not part of formula terms. What matters is only the order of the elements along the dimension. Furthermore, if we require the missing data, What do you and @johnwilkin think of this? Jonathan |
I don't think zlev should be required to have missing values where they are not used. A user might start with a solely z-based coordinate, say, zlev = -5000 (k=1) by 500 to 0 (k=11) and c_depth=0 (or n_sigma=0) but subsequently tinker with increasing values of c_depth and n_sigma. This eats into the entries in zlev and ceases to use some of them (zlev<c_depth) but why demand that unused entries be defined as "missing"? That said, such "tinkering" is not going to be very flexible because it demands a change in the vertical dimension. The number of vertical coordinate surfaces is n_sigma plus the number of z-levels below c_depth. So, if anything, it might be better to require that zlev contain values strictly < c_depth so that the vertical dimension is always length(zlev) plus n_sigma. |
Dear @johnwilkin Thanks for your comment. I think If Best wishes Jonathan |
Quoting my earlier comment
There is another easy way to do this: we should be able to depend on the Jonathan |
I think this suggestion is helpful. The sense of the numbering is readily determined by diff(zlev) or diff(sigma). But if we have sigma dimensioned by |
Hello @johnwilkin and @JonathanGregory, Thanks for these points. I think that the use of the I am now quite troubled by the use It's just occured to me that we dispense with "If the layers are numbered top-down i.e. with With missing values, doesn't "for levels k where sigma(k) has a defined value and zlev(k) is not defined: for levels k where zlev(k) has a defined value and sigma(k) is not defined:" give us all we need? David |
Quite right. I had to sketch this to convince myself. See below. Having vectors Here, the square symbols with the dot are the nominal layer (cell) centers for a typical vertical staggered grid where tracers are defined at the cell centers, and vertical velocity (w) at the layer interfaces dimensioned nlayers+1. To get z of layer interfaces we use the same equation but need a second vector of If the layers are not uniform thickness one can't infer For the sigma-space the layer thicknesses are readily computed by differencing the z on the interfaces computed with sigma_w. |
Dear @davidhassell and @johnwilkin Thanks for your comments, and for the new diagram, John. Omitting the bounds is a problem for many types of vertical coordinate variable, not just this one. Bounds can be provided for both It seems like we agree then. Unless you say otherwise, I'll revise the text to reduce or remove the explicit mention of which way up the coordinates are and the choice of base for Best wishes Jonathan |
Sounds like we've converged and can CLOSE this. And, yes, this doesn't actually require My remarks on "w" grid vertical coordinates is not a bounds issue. It's another coordinate for variables defined at a different location, as in the vertical velocity in this ROMS model dataset
and
u velocity data are dimensioned on s_rho. For ocean_sigma_over_z_coordinate IF the file contains w data, THEN we would need another dimension (length nlayers+1) and coordinate. |
Dear @johnwilkin and @davidhassell I have updated the pull request #317. Is it OK now? Thanks for your help Jonathan |
Thanks, @JonathanGregory. A couple of comments:
|
Dear @davidhassell I have changed the text about I propose that we should keep Best wishes Jonathan |
Dear @JonathanGregory - good points. Keeping |
Another option would be to declare |
Yes, thanks, I agree with Klaus that we should deprecate it, so that a warning is given by the checker. I think we should also check its value, because even if deprecated it may be used by software. Jonathan
|
That sounds good to me, too. |
Dear all I have updated #317 again (at last, I'm beginning to feel that I know what I'm doing with git and github!) to deprecate Best wishes and thanks for your engagement Jonathan |
Thank you, @JonathanGregory. It all looks good to me. |
Hello - the 20th May is here, and no further comments have arisen. Thanks to all for the interesting discussion - and especially to @johnwilkin for the excellent diagrams. Before we merge, however, it is noteworthy that this issue has identified and corrected a fundamental flaw in the conventions (as opposed to a typographical error, for example), which I'm not sure has happened before. The rules on this state The flawed version will be deprecated by a statement in the standard document and the conformance document. However, any data written with the flawed version will not be invalidated, although it may be problematic for users. I am not sure in what form such a statement should take, or in where in the conventions document it should live. In any case, perhaps it should be considered as part PR #317? @JonathanGregory - do you agree the PR should be amended in this way? If so, would you like to propose some extra text? Many thanks, |
Dear @davidhassell According to my computer, the 20th is tomorrow, but I expect you are thinking ahead, or perhaps in Australia? 😄 Thank you for raising this point. I suggest that we insert the following statement just after the title for the "sigma over z" coordinate in Appendix D:
and in the conformance document we add another recommendation for Appendix D:
Best wishes Jonathan |
Dear Jonathan, Oh - what a difference a day makes! Thanks for the text. I think it is fine, but it states that it is still OK to produce CF-1.8 datasets if they don't contain this particular formula - is that what we want? I would have thought that the the creation of all new CF-1.8 datasets should be deprecated. David |
Just listening in here + heard something that affects us, (i.e Iris). I think that disallowing any creation of datasets using older conventions would be problematic for us, likewise maybe anyone who writes generic CF handling code. So, Iris nows support "most" of CF 1.7 for loading : We therfore aim to be CF-1.7 compliant and that is the version level that we state in our output datafiles. So for us, stating CF-1.7 for output data means "you won't find anything in here that is not described by CF-1.7". So, I think it would be unhelpful for us to state compliance with a later version (e.g. 1.8), even though that is "consistent" with our output, if our outputs contain no CF-1.8 -level features. Sorry for hijacking your specific discussion with such a general query. |
Dear @davidhassell and Patrick @pp-mo The rules state that we should deprecate flawed versions of CF, as David says. The aim of doing this is to discourage any more data from being written which will be problematic for users because of the defect which has been discovered. Therefore I'm inclined to think we don't need to deprecate all existing versions of CF to deal with this defect, because it's very limited in scope. We only need to discourage any data from being written with versions <1.9 for the "sigma over z" coordinate. Patrick: The concern of this discussion, which we're resolving with this proposal, is that the text of "sigma over z" in 1.8 and previous versions doesn't describe a correct vertical coordinate variable. If your software supports writing this coordinate on output, the resulting data is flawed. That's not your fault; it's CF's mistake. It doesn't seem like a good idea to continue to write flawed data, does it? It would be better to withdraw support for writing "sigma over z" or to implement the new version. I think it would be fine to call it 1.7 even if you include the 1.9 version of "sigma over z" because it makes more sense than the 1.7 version we are deprecating. Maybe we should say that as well. What do you think? Best wishes Jonathan |
Hi all - The paragraph in the rules document that mentions deprecation seems focused on recent (or even the most recent) changes/versions. The paragraph starts with
This “ocean sigma over z” deprecation would affect all pre-1.9 versions of CF. So there is no change to revoke. How to handle this situation seems unclear, though I lean towards agreement with @JonathanGregory and @pp-mo that it should be more granular than entire versions. Given all that, I think Jonathan’s text looks good. So as not to slow down the resolution of this issue, I propose we move further discussion of the rules around deprecation to a new issue. (I’ll create an issue shortly for further discussion.) |
Should we implement this change without the deprecation, and rely on Ethan's new issue #328 to take care of it later? |
I was a bit confused by how the term deprecation was used by @davidhassell and @JonathanGregory, so I searched for it in the issue, finding that I myself introduced it here. Allow me to clarify how I understand it. Deprecation doesn't apply to versions of an artifact, be it a software package or a standards document. Rather, it applies to specific features. What it says is: "We think this feature should not be used going forward. To allow for a transition period, we do not remove it at this point in time, so you can still use it for a bit, but we'd rather you don't, and we want to remove it in a later version." In my mind, it does not retroactively declare past versions wrong, and writing a new file today that declares that it follows the CF conventions version 1.6 is perfectly legal, if ill-advised. Independent of any deprecation, we might want to have a recommendation to always use the latest version of CF available for new developments. |
Thanks for your clarifications @JonathanGregory and @zklaus. |
I was suggesting moving forward with the deprecation language for this issue and revisiting it once item #328 comes to a conclusion. There are a few other deprecation items currently in the spec (I'll add a list of those in issue #328 shortly) that we'll have to review as well. So adding the deprecation language now would mean all the current deprecations are in one place. But I'm fine either way if waiting on this deprecation language seems a cleaner approach. |
I have updated the pull request #317 to include the proposed deprecation text. If there are no further comments, it can be accepted in three weeks beginning from five days ago when I proposed it. That makes 9th June. |
That's good for me, thanks @JonathanGregory |
The three weeks have passed without comment. Please could you merge the pull request, @davidhassell ? |
Title
Correction to the definition of "ocean sigma over z coordinate" in Appendix D
Moderator
None yet
Moderator Status Review [last updated: YYYY-MM-DD]
None yet
Technical Proposal Summary
The convention for this parametric vertical coordinate appears to be defective in its design. Correcting it cannot be treated as a remedy for a defect, because the intention isn't clear, but a remedy is needed because at the moment there is no definition of what the coordinate variable should contain.
Associated pull request
#317
Detailed Proposal
With the ocean sigma over z coordinate, above depth
depth_c
there arensigma
layers and below this depth the levels are surfaces of constant heightzlev
(positive upwards) relative to the horizontal datum. The vertical coordinatez
for levelk
at timen
and point(j,i)
iswhere
z(n,k,j,i)
is height (positive upwards) relative to the datum (e.g. mean sea level) at gridpoint(n,k,j,i)
,eta(n,j,i)
is the height of the sea surface (positive upwards) relative to the datum at gridpoint(n,j,i)
,sigma(k)
is the dimensionless coordinate at vertical gridpoint(k)
fork <= nsigma
, anddepth(j,i)
is the distance (a positive value) from the datum to the sea floor at horizontal gridpoint(j,i)
.In the above, "
sigma(k)
is the dimensionless coordinate fork <= nsigma
" means thatsigma
is the value that appears for levelk
in the vertical coordinate variable, whose standard name isocean_sigma_z_coordinate
. However,sigma
is not defined fork>nsigma
, so it cannot supply the vertical coordinate value at these lower levels. I think this must be an oversight in the convention as it stands. The convention is therefore faulty in not defining a monotonic set of values for the vertical coordinate variable.I propose that we should delete the statement "
sigma(k)
is the dimensionless coordinate fork <= nsigma
", and add the following:We should also add "The
standard_name
forsigma
isocean_sigma_coordinate
" at the end of the entry, where the other standard names are defined.It would be particularly useful to have comments from anyone who uses this vertical coordinate variable.
The text was updated successfully, but these errors were encountered: