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

Add lambert azimuthal equal area coord_system #1738

Merged
merged 2 commits into from
Oct 10, 2016

Conversation

lbdreyer
Copy link
Member

X offset from planar origin in metres.

* false_northing
Y offset from planar origin in metres.
Copy link
Member

Choose a reason for hiding this comment

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

Please could you add "Defaults to 0." for these arguments.

@rhattersley
Copy link
Member

👍 Now it just needs a new Cartopy release...

@lbdreyer
Copy link
Member Author

lbdreyer commented Aug 5, 2015

It probably also needs an entry to the what's new.

@rhattersley
Copy link
Member

It probably also needs an entry to the what's new.

Yes, good point. 👍

@lbdreyer lbdreyer force-pushed the lambert-equal branch 3 times, most recently from 951b4a3 to 5441678 Compare September 28, 2016 13:09
@lbdreyer
Copy link
Member Author

@bjlittle , this is the pull request I was talking about that is now needed.

(iris.coord_systems.TransverseMercator,
iris.coord_systems.LambertAzimuthalEqualArea)):
units = 'm'

Copy link
Member Author

@lbdreyer lbdreyer Sep 28, 2016

Choose a reason for hiding this comment

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

This block was removed in PR #1943 but I don't agree that it should have been removed.
Section 4.1 and 4.2 of the cf conventions says:

Coordinates of latitude with respect to a rotated pole should be given units of degrees

also 'm' is a perfectly fine unit (see the example in this section).

Copy link
Member

Choose a reason for hiding this comment

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

@lbdreyer Interesting that swapping this back doesn't cause any test failures ...

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, if you look at Example 5.6, that's a rotated_lat_lon grid with units of "degrees". Also Example 5.10 is a tranverse_mercator grid with units of "m".

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I think I must have gotten confused trying to rebase a PR that was over a year old!

On closer inspection I now see that, in the case of tranverse_mercator we're just passing the units from the coord. Although this has it's disadvantages (i.e. it means that a user could produce a netcdf file with a badly defined coordinate:
e.g. DimCoord(array([30, 60]), standard_name='latitude', units=Unit('m'), coord_system=RotatedGeogCS(10.0, 20.0))
But I would prefer for users to have this flexibility, and cf says that there is no default value for lat or lon units.

As the CF conventions recommend a specific unit for GeogCS and RotatedGeogCS I think you could argue that we should 'enforce' these:

The recommended unit of latitude is degrees_north

and

Coordinates of latitude with respect to a rotated pole should be given units of degrees

@lbdreyer
Copy link
Member Author

Failing because of this:
ERROR: test_365_calendar (iris.tests.unit.fileformats.pp_rules.test_convert.TestLBTIM)
but I didn't touch that area of code 😕

@ajdawson
Copy link
Member

That failure was fixed in #2144, targeted at the v1.10.x branch. This change needs to make its way onto master, and then you can rebase onto that to get passing tests here (or we can trust that this feature doesn't cause the failure). There is another test failure too (an error actually), not sure what that one is caused by.

@lbdreyer
Copy link
Member Author

lbdreyer commented Sep 30, 2016

I am going to add a test for the coordinate units discussed in the above comment (I'm hoping that's not a controversial change), and then rebase.

def check(self, coord_system, units, expected_units):
coord = iris.coords.DimCoord([30, 45], 'latitude', units=units,
coord_system=coord_system)
saver = Saver('filename', 'NETCDF4')
Copy link
Member Author

Choose a reason for hiding this comment

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

Argh! This is wrong. I was going to mock it out.

I had made _cf_coord_identity a static method in the hopes that I wouldn't have to instantiate the Saver class to test it, but I was getting an error

>>> coord = iris.coords.DimCoord([45], 'latitude', units='m')
>>> r = Saver._cf_coord_identity(coord)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unbound method _cf_coord_identity() must be called with Saver instance as first argument (got DimCoord instance instead)

@lbdreyer lbdreyer force-pushed the lambert-equal branch 3 times, most recently from 35883f7 to 9e3a3b8 Compare October 3, 2016 13:49
@lbdreyer
Copy link
Member Author

lbdreyer commented Oct 3, 2016

@bjlittle The tests are now passing and the static method is working correctly.

@marqh
Copy link
Member

marqh commented Oct 10, 2016

Hi @lbdreyer

i'm afraid something has occurred on master such that your branch now has conflicts

please rebase and fix so we can see a plausible change with test results

are you still waiting on a cartopy release for this to be able to work properly?

mark

@lbdreyer
Copy link
Member Author

@marqh

I have rebased and the tests are passing again.

are you still waiting on a cartopy release for this to be able to work properly?

No, the change that this required was in the Cartopy 0.14 release

Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

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

Some minor changes, and some feedback from @djkirkham and @marqh would be welcome regarding rotated pole and (CF spec) units of degrees

(iris.coord_systems.TransverseMercator,
iris.coord_systems.LambertAzimuthalEqualArea)):
units = 'm'

Copy link
Member

Choose a reason for hiding this comment

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

@lbdreyer Interesting that swapping this back doesn't cause any test failures ...

@@ -1327,7 +1328,10 @@ def _cf_coord_identity(self, coord):
elif coord.standard_name == "longitude":
units = 'degrees_east'

return (coord.standard_name, coord.long_name, units)
elif isinstance(coord.coord_system, iris.coord_systems.RotatedGeogCS):
units = 'degrees'
Copy link
Member

Choose a reason for hiding this comment

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

@lbdreyer As per the CF spec sections 4.1 and 4.2.

@djkirkham and @marqh This reverts your changes from #1943. Can we all agree this is what we want for rotated pole units, as per CF.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly, we took this out because the units may be radians, say, so changing to degrees would be incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @djkirkham, that makes sense. I will remove this.

self.semi_major_axis = 6377563.396
self.semi_minor_axis = 6356256.909
self.ellipsoid = GeogCS(self.semi_major_axis, self.semi_minor_axis)
self.laea_cs = LambertAzimuthalEqualArea(
Copy link
Member

Choose a reason for hiding this comment

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

@lbdreyer Add in the false_easting and false_northing ... thanks!

def check_call(self, coord_system, units, expected_units):
coord = iris.coords.DimCoord([30, 45], 'latitude', units=units,
coord_system=coord_system)
result = Saver._cf_coord_identity(coord)
Copy link
Member

Choose a reason for hiding this comment

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

@lbdreyer Hence the need for @staticmethod, okay ... 😄

class Test__cf_coord_identity(tests.IrisTest):
def check_call(self, coord_system, units, expected_units):
coord = iris.coords.DimCoord([30, 45], 'latitude', units=units,
coord_system=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.

@lbdreyer Ideally, to give full coverage, could you also test longitude also ... thanks!

self.check_call(coord_system=crs, units='degrees',
expected_units='degrees')

def test_crs_with_no_default_units(self):
Copy link
Member

Choose a reason for hiding this comment

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

@lbdreyer This is really a pass-thru rather than default behaviour ... what do you think?

latitude_of_projection_origin=52,
longitude_of_projection_origin=10,
false_easting=100,
false_northing=200)
Copy link
Member

Choose a reason for hiding this comment

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

@lbdreyer Did you want to pass-thru a choice of ellipsoid here ...

@bjlittle
Copy link
Member

@lbdreyer Great work! Thanks 👍

@bjlittle bjlittle merged commit c577f76 into SciTools:master Oct 10, 2016
@QuLogic QuLogic added this to the v1.11 milestone Oct 11, 2016
@lbdreyer lbdreyer deleted the lambert-equal branch July 23, 2018 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants