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

Correct handling of NetCDF VerticalPerspective and Geostationary coordinate systems #3406

Merged
merged 18 commits into from
Sep 30, 2019

Conversation

trexfeathers
Copy link
Contributor

Addresses #3382

lib/iris/coord_systems.py Outdated Show resolved Hide resolved
lib/iris/coord_systems.py Outdated Show resolved Hide resolved
lib/iris/coord_systems.py Outdated Show resolved Hide resolved
lib/iris/tests/unit/coord_systems/test_Geostationary.py Outdated Show resolved Hide resolved
lib/iris/tests/unit/coord_systems/test_Geostationary.py Outdated Show resolved Hide resolved
@trexfeathers
Copy link
Contributor Author

Still needs a What's New

@pp-mo pp-mo assigned pp-mo and unassigned pp-mo Sep 26, 2019
@lbdreyer lbdreyer removed their assignment Sep 26, 2019
@@ -44,6 +45,10 @@


class Test_write(tests.IrisTest):
# -------------------------------------------------------------------------
# It is not considered necessary to have integration tests for saving
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refers to integration tests, but this is in iris/tests/unit . Can these still be described as integration tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I hadn't considered how oddly this reads. But the tests below (e.g. test_mercator) definitely don't fit the definition of unit test in my opinion since NetCDF saving relies on a large number of steps. Could you suggest a better word than integration, which still makes it clear that these aren't unit tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking a very narrow view, just omit the word "integration" : job done!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, having read through the tests I think I agree that these are closer to integration tests than unit tests. If you want to avoid confusion, it could be worth considering just getting rid of the word "integration", though I'm more convinced now that this ought to be be fine as is.

* :class:`iris.coord_systems.VerticalPerspective` coordinate system now uses
the `CF Vertical perspective definition <http://cfconventions
.org/cf-conventions/cf-conventions.html#vertical-perspective>`_; had been
erroneously using Geostationary.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not be a bug fix rather?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -645,6 +639,104 @@ def as_cartopy_projection(self):
return self.as_cartopy_crs()


class Geostationary(CoordSystem):
"""
An geostationary satellite image map projection.
Copy link
Member

@lbdreyer lbdreyer Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An geostationary -> A geostationary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without reading I copied from vertical perspective, which had been copied without reading from another coord system, so I managed to double the error! I feel appropriately ashamed 😖

@@ -553,7 +555,7 @@ def as_cartopy_projection(self):

class VerticalPerspective(CoordSystem):
"""
An geostationary satellite image map projection.
An vertical/near-side perspective satellite image map projection.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An vertical.. -> A vertical

@@ -563,7 +565,7 @@ def __init__(self, latitude_of_projection_origin,
longitude_of_projection_origin, perspective_point_height,
false_easting=0, false_northing=0, ellipsoid=None):
"""
Constructs an Vertical Perspective Geostationary coord system.
Constructs an Vertical Perspective coord system.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an -> a

@pp-mo
Copy link
Member

pp-mo commented Sep 27, 2019

Needs rebase. Can you do @trexfeathers ?

@trexfeathers
Copy link
Contributor Author

Note that an Iris Geostationary coordinate system will only successfully plot using Cartopy >=0.17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants