Short-cut when transforming PlateCarree -> PlateCarree in mpl. #260

Merged
merged 5 commits into from Apr 12, 2013

Projects

None yet

2 participants

@pelson
Member
pelson commented Apr 8, 2013

Speeds up one PlateCarree contour from 15s to 8s.

A selection of performance test user-code will be available in the near future (not this PR).

@pelson pelson closed this Apr 8, 2013
@pelson pelson reopened this Apr 8, 2013
@pelson pelson commented on an outdated diff Apr 8, 2013
lib/cartopy/mpl/geoaxes.py
x_limits = projection.x_limits
- bypass = x.min() >= x_limits[0] and x.max() <= x_limits[1]
+ y_limits = projection.y_limits
+ bypass = (x.min() >= x_limits[0] and x.max() <= x_limits[1]
+ and y.min() > y_limits[0] and y.max() < y_limits[1])
@pelson
pelson Apr 8, 2013 Member

This change caused the image difference.

@rhattersley
Member

👍 for this change in principle, but there are some detail issues to resolve.

@rhattersley rhattersley and 1 other commented on an outdated diff Apr 10, 2013
lib/cartopy/mpl/geoaxes.py
+
+ src = self.source_projection
+ target = self.target_projection
+ mod = np.diff(target.x_limits)[0]
+
+ verts = src_path.vertices
+ x_lim = verts[:, 0].min(), verts[:, 0].max()
+ y_lim = verts[:, 1].min(), verts[:, 1].max()
+
+ potential = (src.y_limits[0] <= y_lim[0] and
+ src.y_limits[1] >= y_lim[1])
+
+ try:
+ bboxes, proj_offset = target._bbox_and_offset(src)
+ except (ValueError, TypeError):
+ potential = False
@rhattersley
rhattersley Apr 10, 2013 Member

The main issue I have is with the overall structure of this section. The use of the try..except, the private method, and the projection-specific logic (some of which you inherited) all suggest the code is in the wrong place.

I think it would be better if the InterProjectionTransform class knew nothing of _CylindricalProjection or PlateCarree projections. Instead, it should enquire of its projection "Do I really need to transform these vertices? If not, should I use some simple alternative coordinate values instead?".

For example:

bypass_vertices = self.source_projection.can_bypass(self.target_projection, src_path.vertices)
if bypass_vertices is not None:
    if bypass_vertices is not src_path.vertices:
        src_path = mpath.Path(bypass_vertices, src_path.codes)
    return src_path
@pelson
pelson Apr 10, 2013 Member

I'd be happy enough with such an interface, and agree with your evaluation that the code is in the wrong place. Do you want me to do that in this PR?

@rhattersley
rhattersley Apr 10, 2013 Member

Yes please. 😄

@rhattersley
rhattersley Apr 10, 2013 Member

... even if it's just for the PlateCarree subset.

@pelson
Member
pelson commented Apr 10, 2013

@rhattersley - I've now updated this PR.

@rhattersley
Member

I've now updated this PR.

That's much nicer - thank you 😄

I'll have a proper look through now...

@rhattersley rhattersley commented on an outdated diff Apr 10, 2013
lib/cartopy/crs.py
+ to be transformed to wrap appropriately.
+
+ >>> import cartopy.crs as ccrs
+ >>> src = ccrs.PlateCarree(central_longitude=10)
+ >>> bboxes, offset = ccrs.PlateCarree()._bbox_and_offset(src)
+ >>> print bboxes
+ [[-180, -170.0], [-170.0, 180]]
+ >>> print offset
+ 10.0
+
+ The returned values are longitudes in ``other_plate_carree``'s
+ coordinate system.
+
+ """
+ if not isinstance(other_plate_carree, PlateCarree):
+ raise TypeError('pc_proj2 must be a PlateCaree instance.')
@rhattersley
rhattersley Apr 10, 2013 Member

Since this is now a proper private method it'd be simpler to drop this check now. (But if it must stay, pc_proj2 looks to be a legacy term and should be other_plate_carree.)

@rhattersley rhattersley commented on an outdated diff Apr 10, 2013
lib/cartopy/crs.py
+ coordinate system.
+
+ """
+ if not isinstance(other_plate_carree, PlateCarree):
+ raise TypeError('pc_proj2 must be a PlateCaree instance.')
+
+ self_params = self.proj4_params.copy()
+ other_params = other_plate_carree.proj4_params.copy()
+ self_params.pop('lon_0'), other_params.pop('lon_0')
+ if self_params != other_params:
+ raise ValueError('All proj4 params (other than lon_0) of '
+ '"other_plate_carree" must be equal to '
+ 'those of self.')
+
+ central_lon_0 = self.proj4_params['lon_0']
+ central_lon_1 = other_plate_carree.proj4_params['lon_0']
@rhattersley
rhattersley Apr 10, 2013 Member

There's a confusing double meaning for _0 here, especially when combined as lon_0. I suggest you stick with the self vs other convention you've used earlier. For example:

  • central_lon_0 -> self_lon_0
  • central_lon_0_offset -> lon_0_offset
@rhattersley rhattersley and 1 other commented on an outdated diff Apr 10, 2013
lib/cartopy/crs.py
+
+ # Optimise the PlateCarree -> PlateCarree case where no
+ # wrapping or interpolation needs to take place.
+ if return_value is None and isinstance(src_crs, PlateCarree):
+ mod = np.diff(src_crs.x_limits)[0]
+
+ x_lim = vertices[:, 0].min(), vertices[:, 0].max()
+ y_lim = vertices[:, 1].min(), vertices[:, 1].max()
+
+ potential = (self.y_limits[0] <= y_lim[0] and
+ self.y_limits[1] >= y_lim[1])
+
+ try:
+ bboxes, proj_offset = self._bbox_and_offset(src_crs)
+ except (ValueError, TypeError):
+ potential = False
@rhattersley
rhattersley Apr 10, 2013 Member

Using a try..except block here still seems odd when it's really just normal logic flow. Perhaps the structure in this function should be:

if PlateCarree -> PlateCarree and all params matching except lon_0:
    Compute bboxes and proj_offset (now with no failure modes)
    Check if vertices sit inside bboxes (as before)
@pelson
pelson Apr 12, 2013 Member

I'm not keen on this. If anybody else wants to make use of _bbox_and_offset they will also have to do this checking - also, without running the same tests in _bbox_and_offset there is no guarantee that its result is sensible. It is for that reason that I'm comfortable with a try...except block inline here.
Obviously, if you insist I will go ahead and implement the same checks in both methods to remove the try...except - but I'm not keen for _bbox_and_offset to avoid raising Exceptions.

@rhattersley
rhattersley Apr 12, 2013 Member

The bit I really care about is you shouldn't catch the TypeError. You've already checked the type so you don't expect a TypeError to be raised, so if one does crop up it's a genuine error and shouldn't be silenced.

After that, I still think the use of the ValueError suggests a more elegant refactor is possible ... but I'm not going to get hung up on that.

@rhattersley rhattersley and 1 other commented on an outdated diff Apr 10, 2013
lib/cartopy/tests/mpl/test_axes.py
+
+ # of the 3 paths, 2 of them cannot be short-cutted.
+ pth1 = mpath.Path([[0.5, 0], [10, 10]])
+ pth2 = mpath.Path([[0.5, 91], [10, 10]])
+ pth3 = mpath.Path([[-0.5, 0], [10, 10]])
+
+ trans = InterProjectionTransform(src, target)
+
+ counter = CallCounter(target, 'project_geometry')
+
+ with counter:
+ trans.transform_path(pth1)
+ trans.transform_path(pth2)
+ trans.transform_path(pth3)
+
+ assert_equal(counter.count, 2)
@rhattersley
rhattersley Apr 10, 2013 Member

By grouping them we don't know which one used the short-cut.

@pelson
pelson Apr 10, 2013 Member

No, but it doesn't really matter. I don't even test that the result is correct. It is purely an "is this optimised" test. There are other tests which will pick up badly projected polygons.

@pelson
pelson Apr 12, 2013 Member

FWIW I've updated this test to be more specific.

@rhattersley
rhattersley Apr 12, 2013 Member

Thanks - that clarifies which paths should be optimised, which might be handy if/when we ever come revisit this.

@rhattersley rhattersley commented on an outdated diff Apr 10, 2013
lib/cartopy/tests/test_crs.py
+ ([[-180, -170], [-170, 180]], 10),
+ ([[-180, 170], [170, 180]], -10),
+ ([[-180, 180], [180, 180]], 360),
+ ([[-180, -180], [-180, 180]], -360),
+ ]
+
+ assert len(target) == len(central_lons)
+
+ for expected, (s_lon0, t_lon0) in zip(target, central_lons):
+ expected_bboxes, expected_offset = expected
+
+ src = ccrs.PlateCarree(central_longitude=s_lon0)
+ target = ccrs.PlateCarree(central_longitude=t_lon0)
+
+ bbox, offset = src._bbox_and_offset(target)
+ print s_lon0, t_lon0, bbox, offset
@pelson
Member
pelson commented Apr 12, 2013

I've done all actions bar the one I've commented on. Lets thrash this out and merge in the next 2 hours 😉.

Thanks for the review @rhattersley.

@pelson
Member
pelson commented Apr 12, 2013

Just added a commit which addresses the try except.

@rhattersley
Member

Nice work @pelson 😄

It's faster and it's cleaned out some hacky code from the InterProjectionTransform. 👍

@rhattersley rhattersley merged commit 767b85d into SciTools:master Apr 12, 2013

1 check passed

default The Travis build passed
Details
@pelson
Member
pelson commented Apr 12, 2013

Thanks @rhattersley .

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