Skip to content
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

Fix geostationary projection #230

Closed
erget opened this issue Jan 14, 2020 · 26 comments
Closed

Fix geostationary projection #230

erget opened this issue Jan 14, 2020 · 26 comments
Assignees
Labels
defect Conventions text meaning not as intended, misleading, unclear, has typos, format or language errors

Comments

@erget
Copy link
Member

erget commented Jan 14, 2020

Title: Fix geostationary projection

Moderator: @JimBiardCics

Requirement Summary: We should describe the geolocation data from geostationary satellites correctly. This is not currently the case.

Technical Proposal Summary: Change the error in the Conventions, deprecate the usage of the previous standard names associated with that in the context where they are incorrect, and add 2 standard names to be able to describe coordinates in such data correctly.

Benefits: Users who want to have a correct understanding of coordinates in geostationary products without building special cases into their software.

Status Quo:
The current geostationary projection uses projection_x_coordinate and projection_y_coordinate to encode coordinates. Their units in the projection are described in Appendix F as radians. However, the standard_name attributes have meters as their canonical units. According to Section 3.3 if a standard_name uses different units than the canonical units, these must be physically equivalent. This is not the case for coordinates as used in geostationary products. This can lead to confusion.

Detailed Proposal: This had been discussed previously on the mailing list but seems to have fizzled out. The current iteration of the proposal had support from @erget @ethanrd and @JonathanGregory but should be reviewed again after laying dormant since April 2019.

Scope of changes:

@erget erget added the defect Conventions text meaning not as intended, misleading, unclear, has typos, format or language errors label Jan 14, 2020
@steingod
Copy link

I support this proposal to achieve a consistent approach.

@lllliso
Copy link

lllliso commented Jan 14, 2020

I fully support this proposal, I work in the NWCSAF http://nwc-saf.eumetsat.int/, for the NWCSAF GEO part this inclusion is really convenient. This inclusion will highly benefit the users' community

Llorenç Lliso
PM Deputy of the NWCSAF

@JimBiardCics
Copy link
Contributor

I also support the proposal.

@cf-metadata-list
Copy link

cf-metadata-list commented Jan 14, 2020 via email

@JonathanGregory
Copy link
Contributor

I'm in favour as well. Thanks.

@magau
Copy link

magau commented Jan 15, 2020

I'm not sure if I totally agree with the proposal...

I've been working on NetCDFs with geostationary projection for a while and have made my own contribution in the GDAL library and ADAGUC Server, in order to support "radian" units for the geostationary projection coordinates/dimension:

OSGeo/gdal#1799
KNMI/adaguc-server#122

and Proj4 Documentation review (sweep angle):

OSGeo/PROJ#1179

I also know that the netcdf-java library supports "radian" units for geostationary projection. Besides, it searches for the projection_x_coordinate and projection_y_coordinate values of the standard_name attribute to identify the x\y horizontal coordinates variables:
https://github.com/Unidata/netcdf-java/blob/master/cdm/core/src/main/java/ucar/unidata/geoloc/projection/sat/Geostationary.java#L33
The netcdf-java library is used by a bunch of NetCDF visualization softwares like: Panoply, McIDAS, THREDDS Data Server, etc...
Thus, if your proposal is accepted I gess you'll loose a lot of software compatibility.

My question is:
Is it possible to change the projection_x_coordinate and projection_y_coordinate standard names or even the standard_name definitions, in order to keep both units, "m" and "radian", compatible with the horizontal coordinates variable?
Consider that you can obtain the geostationary coordinates in meters, from radians, multiplying it by the perspective_point_height attribute value.

From my point of view, if the longitude and latitude values could be replaced by the projection_x_coordinate and projection_y_coordinate, the horizontal coordinates/dimensions could be generalized to be easily identified on any dataset, independently of the projection.
And maybe It could also be interesting, concerning this generalization, to have the vertical_coordinate standard_name, so all the spatial coordinates could be differentiated from "time", "reference_time" or other possible dimensions, independently of its order.

Hope I have understood this proposal correctly, because I believe it will increase the number of exceptions on the horizontal coordinates/dimension variables identification process and that it can have farther impacts...

@mraspaud
Copy link
Contributor

I'm partly responsible for having the projection coordinates in radians today, but I support this proposal. I would suggest that the units of the projection_x/y_coordinate would be used in the libs mentionned by @magau to determine if the values need to be multiplied by the perspective_point_height or not.

@erget
Copy link
Member Author

erget commented Jan 16, 2020

@magau thanks for the input. I realise that this touches current practices. Unfortunately, the current wording in the document is inconsistent; whereas most people are using radians or another angular unit to encode coordinates in geostationary projections, and this is supported by current software, the canonical unit of the coordinate variable is linear and thus cannot be converted linearly into the units specified in the data products. Although you're right about multiplying by perspective_point_height this is a parametrised unit transform at best, so IMHO it doesn't really imply physical equivalence between the units as required by the CF Conventions' wisdom on standard names.

So the impacts as I see it are (for people who encode geostationary products):

  • if you're using a CF version <=1.7, none
  • if you migrate to use a CF version >1.7 you replace the projection_x_coordinate and projection_y_coordinate with projection_x_angular_coordinate and projection_y_angular_coordinate. Or you don't change your product and accept that you are using a deprecated practice that may not be allowed in future versions of the CF Conventions (1.9 or higher).

@erget
Copy link
Member Author

erget commented Jan 17, 2020

I've implemented the discussed changes in the referenced PR. Anyone like to step in as moderator and start the clock/debate?

@JimBiardCics
Copy link
Contributor

@erget I'd moderate.

@erget
Copy link
Member Author

erget commented Jan 17, 2020

Thanks @JimBiardCics ! I was digging through the rules and wasn't able to find out what deadlines apply to corrections vs. enhancements (I'd consider this a correction, but others may disagree). The 1.8 release will probably be early February this year, should we try to vector this in there? If not I'll need to adjust the wording to reflect that it's deprecated from v1.9.

@JimBiardCics
Copy link
Contributor

Deferred to v1.9? That's probably a good plan.

@erget
Copy link
Member Author

erget commented Jan 17, 2020

PR updated.

@JimBiardCics JimBiardCics self-assigned this Jan 17, 2020
@JimBiardCics
Copy link
Contributor

Here's a summary of the issue to date.

New standard names to be used with coordinates variables for the geostationary projection have been proposed to resolve a problem with canonical units that exists with the standard names being used to date. This problem is a defect in the current version of CF (CF 1.7).

The current CF conventions specify that the rectangular coordinates for the geostationary projection are identified by the standard_name attributes projection_x_coordinate and projection_y_coordinate. The standard name table declares these standard names to have canonical units of meters. The CF convention regarding standard names specifies that the units for a variable with a given standard name must have units that are convertible to the canonical units for that standard name. This conversion must be a simple units conversion — length to length, volume to volume, etc.

The coordinate values for the geostationary projection are angles and have units of degrees or radians. It is a defect to use standard_name attributes of projection_x_coordinate and projection_y_coordinate for such coordinate variables because units of angles are not directly convertible to units of length.

The proposed correction for the defect is creation of two new standard names for use with the geostationary projection. The new standard names are projection_x_angular_coordinate and projection_y_angular_coordinate. These new names would have canonical units of radians.

A number of commenters have said they approve of the change. Others have expressed reservations about impacts on those who have already implemented software based on the current version of CF that might not recognize the coordinate variables because it was expecting the old standard names. @magau asked if it might be possible to change the CF conventions regarding standard name canonical units or make some exception for these particular standard names to resolve the defect without specifying new standard names.

@erget suggested that the reservations, while valid, should not prevent correction of the defect, as it would not invalidate files claiming adherence to CF 1.7 (or 1.8), and it should not be difficult for software to support the new standard names.

People indicating approval so far are:
@lllliso
@JimBiardCics
@steingod
@JonathanGregory
Randy Horne
@mraspaud

People indicating concerns so far are:
@magau

@cameronsmith1
Copy link

cameronsmith1 commented Jan 17, 2020

I support the proposal. I understand that this will be a nuisance for software reading the existing files that use angles instead of distance. However, I too think it should be easy enough to modify the existing code. Another option would be for those users to continue using the older CF version for their data, although I would hope they switch to the newer CF version.

@lesserwhirls
Copy link
Contributor

This seems mostly to be about making the CF Metadata Conventions consistent. @JimBiardCics provided an excellent summary.

With this proposal, it appears the units used by the variable containing the map coordinates remains the same before and after the change (both use radians), and none of the projection math changes. That's a win in terms of "not much work", I think. This proposal isn't suggesting we start allowing the map coordinate variable to use meters in addition to radians, is it? One might get that impression (see https://github.com/cf-convention/cf-conventions/pull/232/files#r368206817). That kind of change could might add up to some work. But assuming that's not the case, we're basically looking at a change in value of the standard_name attribute to prevent the CF document from becoming CF-ish. In this specific case (standard_name of a coordinate variable containing map coordinates), what practically, is the standard_name attributes use?

Already TL;DR;? Ok, I'll cut to the chase - I'm in favor of this change (for what it's worth).

I'll say up front that being consistent is a good thing on its own, even if adds to my own pile of work :-) So again, the question is, what kind of work would be created by this change. There are two perspectives from which we can look at this: reading vs writing. And to make things painful, I'll bring up netCDF-Java in particular, because that's the implementation I know (I think). Your mileage may vary.

Impact on reading

For reading purposes, I'm not sure what the standard_name and its specific value it would be used for outside of compliance checking. I guess it could be used to identify a potential coordinate variables (hello there, strawman). However, CF does not appear to suggest that coordinate variables should ever be identified in using the standard_name attribute, but through either:

  1. the NUG definition (names of a 1D variable and dimension match), or
  2. the variable name appearing in the coordinates attribute.

This is what netCDF-Java does when it encounters a global attribute Conventions with string value partially matching CF-1. (and others cases, but for sure CF). Because of the Geostationary projection definition in the CF document, we (netCDF-Java) assume the map coordinates are some in form of radians and we run with it (scissors in hand). We never look at the standard_name attribute in this case. Since that assumption does not change with this proposal, the netCDF-Java library is good to go.

Now, code might be looking at the standard_name attribute as an attempt to be flexible (a worthy effort, except for a compliance checker, for sure), but that's outside of the CF specification as far as I can tell.

So, for reading purposes (outside of a compliance checker use case), the question is, what does the standard_name actually provide in practice? That is:

if (standard_name == "projection_x_coordinate":
    // then what?

Honest question, and any insights would be helpful to the discussion, I believe. As as Billy the Kid (played by Emilio Estavez) said in the movie Young Guns, "[t]here's many a slip twixt the convention and the implementation" (I think that's how it goes...), and I'm only looking from the point of view of one implementation.

Impact on writing

For writing, then it becomes a little more tricky, but only if you want to write a file with a global attribute Conventions whose string value is CF-1.9 or above. At that point, you've already updated the code to bump the CF version being written out, so might as well update the standard_name attribute on the variable holding the map coordinate data to use the newer _angular_ flavor. The actual data in the array does not change, the unit does not change, just the value of the standard_name attribute attached to the variable. This will impact netCDF-Java directly, but only slightly. When we write data to CF compliant netCDF files, we try to make sure the required CF attributes are attached to the variables holding the map coordinate data. Currently, we check to see if we are dealing with a coordinate variable holding map coordinate data, and if so, we make sure it has a standard_name attribute with a value of projection_x_coordinate or projection_y_coordinate. After this change, we will need to add a few lines to check what the Conventions attribute says, and if 1.9 or greater, check the units on the variable. If the units are compatible with meters, then use projection_x_coordinate, if compatible with radians, use the new projection_<x|y>_angular_coordinate values. We could get more picky and only allow the _angular_ version for geostationary projections - we'll see as the discussion on this issue continues. As far as I can tell, nothing else changes on my end.

Again, for what it's worth, at this point I'm +1 on this change.

@magau
Copy link

magau commented Jan 18, 2020

Since I've been the only one raising concerns about the proposal, which I'm a bit shame of, I must say that I agree with most of all that has been said by all of you.

Actually, I'm in contradiction with myself activities since, has I've said before, I've been putting some efforts for the radian units, for the geostationary projection, to be supported by other software than the java-netcdf. But what I really think, is that nothing of this would be necessary if the geostationary coordinates units were defined in meters. Which is, I guess, what Proj library is using, and also GDAL for writing. As most of the projected coordinates systems (safeguarding exceptions like the equirectangular projection and derived using latitude/longitude...).

Like @erget said, and I totally agree, for those who are encoding netcdf files, as data producers, the impact of this changes will be minimum.
My concerns goes for those who are, or that may be in future, developing automated processes for making projection transforms and post writing.
Maybe It would be good that they have less exceptions to handle with...

Any way, I totally agree that what has been discussed here is effectively a defect and it has to be solved. So, it won't be me who'll keep feeding the discussion.

I'd also like to thanks to all of you that are participating and are giving their contribute for a better understanding of the implications of this change, in a way that the cf-conventions can keep responding to the CF community needs.

@erget
Copy link
Member Author

erget commented Jan 20, 2020

@magau your input is very valuable and there's no reason that you'd have to be ashamed of any part of it - for such a widely used standard as CF there is a need for a diversity of perspectives in order to catch all the ramifications of changes like this and thus it's very much appreciated!

@lesserwhirls
Copy link
Contributor

Totally agree with @erget - I see no reason to be ashamed, and your input and perspective is certainly appreciated! @magau - would you happen to know of an easy to access dataset with the map coordinates in m instead of radians?

@cameronsmith1
Copy link

@magau your input is very valuable and there's no reason that you'd have to be ashamed of any part of it - for such a widely used standard as CF there is a need for a diversity of perspectives in order to catch all the ramifications of changes like this and thus it's very much appreciated!

I also agree with @erget . It is important that CF doesn't push for purity at the expense of users, so thank you for speaking up.

@davidhassell
Copy link
Contributor

I'm just catching up on this, and I also support the proposal as it stands.

@JimBiardCics
Copy link
Contributor

It appears that there is enough support for this proposal to start the countdown clock for acceptance of change #232. If there are no substantive modifications or objections, it can be merged in three weeks (February 13, 2020) for inclusion in CF 1.9 per the contribution guidelines.

@erget
Copy link
Member Author

erget commented Feb 13, 2020

Just a note concerning the pending merge of the PR corresponding to this issue - it is also attached to #231, which seems uncontroversial and has not generated any discussion.

@snowman2
Copy link
Contributor

Potential alternative in #248

@erget
Copy link
Member Author

erget commented Feb 19, 2020

Hi @JimBiardCics is this ready to merge? The three weeks have passed.

@JimBiardCics
Copy link
Contributor

@erget I believe it is. I was out of town.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect Conventions text meaning not as intended, misleading, unclear, has typos, format or language errors
Projects
None yet
Development

No branches or pull requests