Add support for loading NetCDF files with Mercator projections #2026
Conversation
djkirkham
force-pushed
the
add-netcdf-mercator
branch
from
a3af0c0
to
d35aba9
Compare
|
Just had a thought. When loading a NetCDF file with a Mercator projection where the values for the restricted parameters do not match the defaults, an exception is raised. Previously, any NetCDF file defining a Mercator projection would still be loaded, without the projection and containing anonymous coordinates for x and y. So this change provides less functionality than before. Perhaps we should change the errors to warnings, and not load the projection? |
|
|
||
| """ | ||
|
|
||
| #: True longitude of planar origin in degrees. |
There was a problem hiding this comment.
Comments should be structured as # My comment here. please 😄
There was a problem hiding this comment.
These are "Doc Comments" for documenting the attributes in Sphinx.
There was a problem hiding this comment.
Let's pretend I knew that and didn't just copy the comments from another class...
There was a problem hiding this comment.
If you don't want them documented (probably no need, but I don't really mind) then remove the :.
There was a problem hiding this comment.
I'll leave them as they are
djkirkham
force-pushed
the
add-netcdf-mercator
branch
from
b592e40
to
eeb7325
Compare
|
|
||
| def __repr__(self): | ||
| return "Mercator(central_lon={!r}, "\ | ||
| "ellipsoid={!r})".format( |
There was a problem hiding this comment.
Alternatively (and a pattern I personally prefer):
res = 'Mercator(central_longitude={!r}, ellipsoid={!r})'
return res.format(self.central_longitude, self.ellipsoid)Saves the slightly forced splitting of lines. I'm also assuming you've changed the name of central_longitude 😉
|
I'm generally in favour of being permissive on load. I don't think we want to be throwing an exception if we can help it. Partially loading the data allows the user to fill in any missing details manually (perhaps according to some other convention), but throwing an error leaves them stuck. |
| <attribute name="geospatial_lat_min" value="0.0"/> | ||
| <attribute name="geospatial_lon_max" value="0.0"/> | ||
| <attribute name="geospatial_lon_min" value="0.0"/> | ||
| <attribute name="history" value="Created: 2016-05-23T09:40:00Z"/> |
There was a problem hiding this comment.
Is this history attribute going to give us problems with ever-changing values?
There was a problem hiding this comment.
This is the result of a load test, so the attribute doesn't change; it is loaded in from a file.
| def test_merc(self): | ||
| tm = merc() | ||
| expected = "Mercator(longitude_of_projection_origin=90.0, "\ | ||
| "ellipsoid=GeogCS(semi_major_axis=6377563.396, semi_minor_axis=6356256.909))" |
There was a problem hiding this comment.
This line goes over the 80 character limit. I assume that this was not picked up by the coding standards test because this is one of the files skipped by the test. Nevertheless as this is new code it would be good if it obeyed the 80 character limit!
|
Tests pass!! 🎉 I've one last comment (the one just above about the line length) and then after a squash of commits I'm 👍 |
djkirkham
force-pushed
the
add-netcdf-mercator
branch
from
943ebab
to
55021fd
Compare
|
Stupid long-running tests timing out 😑 ⏳ 💥 I'm going to merge this anyway as (a) the tests that are timing out are nothing to do with @djkirkham's changes, and (b) the tests all passed before the squash. Boom. Progress. |
This change supports loading and saving NetCDF files with a Mercator projection with some parameters restricted (
false_easting == false_northing == 0, scale_factor_at_projection_origin == 1). This allows some support for Mercator projections without needing to modify Cartopy.This pull request requires additional test data (SciTools/iris-test-data#42).