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

Trans ndim pnts #192

Merged
merged 3 commits into from Feb 12, 2013

Conversation

Projects
None yet
4 participants
@cpelley
Contributor

cpelley commented Jan 17, 2013

Relates to Iris pull request 313.
(all cartopy tests pass, but one Iris test fails, see link
This pull request enhances the ability of cartopy to handle multidimensional arrays supplied to transform_points

@pelson

View changes

Show outdated Hide outdated lib/cartopy/__init__.py
@@ -16,7 +16,7 @@
# along with cartopy. If not, see <http://www.gnu.org/licenses/>.
__version__ = '0.5.x'
__version__ = '0.5.0'

This comment has been minimized.

@pelson

pelson Jan 17, 2013

Member

Needs reverting. Do this by rebasing your branch against upstream/master.

@pelson

pelson Jan 17, 2013

Member

Needs reverting. Do this by rebasing your branch against upstream/master.

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Jan 21, 2013

Member

Closing until ready for a review. Feel free to re-open when your happy @cpelley.

Member

pelson commented Jan 21, 2013

Closing until ready for a review. Feel free to re-open when your happy @cpelley.

@pelson pelson closed this Jan 21, 2013

@cpelley cpelley reopened this Jan 22, 2013

@cpelley cpelley closed this Jan 22, 2013

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Jan 22, 2013

Member

We definitely need a new cartopy test for the interface. Something as simple as transforming PlateCarree to Geodetic will be fine.

Member

pelson commented Jan 22, 2013

We definitely need a new cartopy test for the interface. Something as simple as transforming PlateCarree to Geodetic will be fine.

@cpelley cpelley reopened this Jan 22, 2013

@cpelley

This comment has been minimized.

Show comment
Hide comment
@cpelley

cpelley Jan 22, 2013

Contributor

Ah ok, thanks @pelson I'm on it!

Contributor

cpelley commented Jan 22, 2013

Ah ok, thanks @pelson I'm on it!

@cpelley cpelley referenced this pull request Jan 22, 2013

Merged

Rm pyproj dep #313

@cpelley

This comment has been minimized.

Show comment
Hide comment
@cpelley

cpelley Jan 23, 2013

Contributor

where would be the most appropriate place to place this test? (test_crs?)

Contributor

cpelley commented Jan 23, 2013

where would be the most appropriate place to place this test? (test_crs?)

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Jan 23, 2013

Member

I'd have said so, yes. Do what you think best though.

Member

pelson commented Jan 23, 2013

I'd have said so, yes. Do what you think best though.

@cpelley

This comment has been minimized.

Show comment
Hide comment
@cpelley

cpelley Feb 6, 2013

Contributor

changes made, squashed and rebased
review required

Contributor

cpelley commented Feb 6, 2013

changes made, squashed and rebased
review required

@ghost ghost assigned pelson Feb 6, 2013

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Feb 6, 2013

Member

Thanks @cpelley - looking into it now.

Member

pelson commented Feb 6, 2013

Thanks @cpelley - looking into it now.

@pelson

View changes

Show outdated Hide outdated lib/cartopy/_crs.pyx
raise ValueError('x and y arrays must be one dimensional')
if x.shape[0] != y.shape[0]:
raise ValueError('x and y arrays must have the same length')
if not x.ndim == y.ndim or x.ndim > 2:

This comment has been minimized.

@pelson

pelson Feb 6, 2013

Member

Would prefer a != b vs not a == b.

@pelson

pelson Feb 6, 2013

Member

Would prefer a != b vs not a == b.

This comment has been minimized.

@cpelley

cpelley Feb 6, 2013

Contributor

I agree, done

@cpelley

cpelley Feb 6, 2013

Contributor

I agree, done

@pelson

View changes

Show outdated Hide outdated lib/cartopy/_crs.pyx
raise ValueError('x and y arrays must have the same length')
if not x.ndim == y.ndim or x.ndim > 2:
raise ValueError(
'x and y arrays must have the same dimension '\

This comment has been minimized.

@pelson

pelson Feb 6, 2013

Member

Is this slash necessary?

@pelson

pelson Feb 6, 2013

Member

Is this slash necessary?

This comment has been minimized.

@cpelley

cpelley Feb 6, 2013

Contributor

change, thanks @pelson

@cpelley

cpelley Feb 6, 2013

Contributor

change, thanks @pelson

This comment has been minimized.

@esc24

esc24 Feb 7, 2013

Member

I find the new wording confusing. I assume you mean the x and y arrays must have the same number of dimensions, or put another way, they must have the same dimensionality.

@esc24

esc24 Feb 7, 2013

Member

I find the new wording confusing. I assume you mean the x and y arrays must have the same number of dimensions, or put another way, they must have the same dimensionality.

@pelson

View changes

Show outdated Hide outdated lib/cartopy/_crs.pyx
* y - the y coordinates (array), in ``src_crs`` coordinates,
to transform
* z - (optional) the z coordinates (array), in ``src_crs``
coordinates, to transform
Returns:

This comment has been minimized.

@pelson

pelson Feb 6, 2013

Member

I believe this return value is no longer correctly documented...

@pelson

pelson Feb 6, 2013

Member

I believe this return value is no longer correctly documented...

This comment has been minimized.

@cpelley

cpelley Feb 6, 2013

Contributor

in what way? still returns transformed arrays

@cpelley

cpelley Feb 6, 2013

Contributor

in what way? still returns transformed arrays

This comment has been minimized.

@pelson

pelson Feb 7, 2013

Member

What shape do you expect the result to be. Is it still always (n, 3)? If so, I have an alternative implementation suggestion for this PR which will simplify significantly & avoid the need for multiple copies of data (as has been added by this PR).

@pelson

pelson Feb 7, 2013

Member

What shape do you expect the result to be. Is it still always (n, 3)? If so, I have an alternative implementation suggestion for this PR which will simplify significantly & avoid the need for multiple copies of data (as has been added by this PR).

This comment has been minimized.

@cpelley

cpelley Feb 8, 2013

Contributor

the expected result is either (n, 3) or (n, D, 3) for multidimensional (see tests)
where D is the number of dimension of the input arrays

@cpelley

cpelley Feb 8, 2013

Contributor

the expected result is either (n, 3) or (n, D, 3) for multidimensional (see tests)
where D is the number of dimension of the input arrays

This comment has been minimized.

@pelson

pelson Feb 8, 2013

Member

in what way? still returns transformed arrays

The documentation states:

        Returns:

            array of shape [npts, 3] in this coordinate system

That is no longer true.

@pelson

pelson Feb 8, 2013

Member

in what way? still returns transformed arrays

The documentation states:

        Returns:

            array of shape [npts, 3] in this coordinate system

That is no longer true.

This comment has been minimized.

@cpelley

cpelley Feb 8, 2013

Contributor

oh I see, yes I missed that

@cpelley

cpelley Feb 8, 2013

Contributor

oh I see, yes I missed that

@pelson

View changes

Show outdated Hide outdated lib/cartopy/tests/test_crs.py
def test_transform_points_nD(self):
rlons = numpy.array([[350., 352.], [350., 352.]])
rlats = numpy.array([[-5., -0.], [-4., -1.]])
rdata = numpy.array([[1., 2.], [3., 4.]])

This comment has been minimized.

@pelson

pelson Feb 6, 2013

Member

What is this for?

@pelson

pelson Feb 6, 2013

Member

What is this for?

This comment has been minimized.

@cpelley

cpelley Feb 6, 2013

Contributor

well you can optionally pass the z coordinate into transform_points (just thoroughly testing the interface)
not correct?

@cpelley

cpelley Feb 6, 2013

Contributor

well you can optionally pass the z coordinate into transform_points (just thoroughly testing the interface)
not correct?

This comment has been minimized.

@pelson

pelson Feb 7, 2013

Member

The numbers are not transformed and they don't mean anything. In many respects, we should disallow this functionality if we know we have a 2d projection.

@pelson

pelson Feb 7, 2013

Member

The numbers are not transformed and they don't mean anything. In many respects, we should disallow this functionality if we know we have a 2d projection.

@pelson pelson referenced this pull request Feb 7, 2013

Closed

Multi dim transform points #206

@cpelley

This comment has been minimized.

Show comment
Hide comment
@cpelley

cpelley Feb 8, 2013

Contributor

since @pelson has found a neater solution to this, I propose we switch to PR #206

Contributor

cpelley commented Feb 8, 2013

since @pelson has found a neater solution to this, I propose we switch to PR #206

@cpelley cpelley closed this Feb 8, 2013

@cpelley cpelley reopened this Feb 8, 2013

@cpelley

This comment has been minimized.

Show comment
Hide comment
@cpelley

cpelley Feb 8, 2013

Owner

is this the correct way of dealing with this?

Owner

cpelley commented on lib/cartopy/_crs.pyx in a3c4031 Feb 8, 2013

is this the correct way of dealing with this?

@cpelley

This comment has been minimized.

Show comment
Hide comment
@cpelley

cpelley Feb 11, 2013

Contributor

Ready for further review

Contributor

cpelley commented Feb 11, 2013

Ready for further review

@pelson

View changes

Show outdated Hide outdated lib/cartopy/_crs.pyx
Returns:
array of shape [npts, 3] in this coordinate system
array of shape [npts, 3] in this coordinate system for input

This comment has been minimized.

@pelson

pelson Feb 11, 2013

Member

I'm not a fan of the docstring. I'll see if I can come up with something better (and I think the result shape is not correct either).

@pelson

pelson Feb 11, 2013

Member

I'm not a fan of the docstring. I'll see if I can come up with something better (and I think the result shape is not correct either).

This comment has been minimized.

@pelson

pelson Feb 11, 2013

Member
Returns:

    array of shape ```x.shape + (3, )``` in this coordinate system.
@pelson

pelson Feb 11, 2013

Member
Returns:

    array of shape ```x.shape + (3, )``` in this coordinate system.

This comment has been minimized.

@pelson

pelson Feb 11, 2013

Member

Please also add a seealso to the transform_point method.

@pelson

pelson Feb 11, 2013

Member

Please also add a seealso to the transform_point method.

@ghost ghost assigned rhattersley Feb 11, 2013

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Feb 11, 2013

Member

Assigning to @rhattersley as I wrote some of this code. I'll also review though.

Member

pelson commented Feb 11, 2013

Assigning to @rhattersley as I wrote some of this code. I'll also review though.

* x - the x coordinates (array), in ``src_crs`` coordinates, to transform
* y - the y coordinates (array), in ``src_crs`` coordinates, to transform
* z - (optional) the z coordinates (array), in ``src_crs`` coordinates, to transform
* x - the x coordinates (array), in ``src_crs`` coordinates,

This comment has been minimized.

@pelson

pelson Feb 11, 2013

Member

This may be upto 2 dimensional (an exception occurs after this).

Please add that to the docs of the x parameter (don't need to repeat in y and z).
Also, please catch that limitation and provide a nicer exception than is currently given.

@pelson

pelson Feb 11, 2013

Member

This may be upto 2 dimensional (an exception occurs after this).

Please add that to the docs of the x parameter (don't need to repeat in y and z).
Also, please catch that limitation and provide a nicer exception than is currently given.

This comment has been minimized.

@cpelley

cpelley Feb 11, 2013

Contributor

ok will do thanks @pelson

@cpelley

cpelley Feb 11, 2013

Contributor

ok will do thanks @pelson

@cpelley

This comment has been minimized.

Show comment
Hide comment
@cpelley

cpelley Feb 11, 2013

Contributor

All requested changes made (tested .pyx doc changes with automodule).

Contributor

cpelley commented Feb 11, 2013

All requested changes made (tested .pyx doc changes with automodule).

Show outdated Hide outdated lib/cartopy/_crs.pyx
if x.ndim != 1 or y.ndim != 1 or z.ndim != 1:
raise ValueError('x, y and z arrays must be one dimensional')
if x.ndim > 2 or y.ndim > 2 or z.ndim > 2:
raise ValueError('x, y and z arrays must be 1 or 2'

This comment has been minimized.

@pelson

pelson Feb 12, 2013

Member

Needs a space after the 2.

@pelson

pelson Feb 12, 2013

Member

Needs a space after the 2.

This comment has been minimized.

@pelson

pelson Feb 12, 2013

Member

Does this need to be over 2 lines? 242 suggests not?

@pelson

pelson Feb 12, 2013

Member

Does this need to be over 2 lines? 242 suggests not?

This comment has been minimized.

@cpelley

cpelley Feb 12, 2013

Contributor

yes it does, L:242 is slightly shorter a 77 long

@cpelley

cpelley Feb 12, 2013

Contributor

yes it does, L:242 is slightly shorter a 77 long

Show outdated Hide outdated lib/cartopy/_crs.pyx
if not x.shape[0] == y.shape[0] == z.shape[0]:
raise ValueError('x, y, and z arrays must have the same length')
raise ValueError('x, y, and z arrays must have the same'

This comment has been minimized.

@pelson

pelson Feb 12, 2013

Member

Again. Needs a space. Was the line length greater than 79 chars?

@pelson

pelson Feb 12, 2013

Member

Again. Needs a space. Was the line length greater than 79 chars?

Show outdated Hide outdated lib/cartopy/tests/test_crs.py
res = target_proj.transform_points(x=rlons, y=rlats,
src_crs=src_proj)
unrotated_lon, unrotated_lat, unrotated_dat = res.transpose()

This comment has been minimized.

@pelson

pelson Feb 12, 2013

Member

unrotated_dat -> _

@pelson

pelson Feb 12, 2013

Member

unrotated_dat -> _

Show outdated Hide outdated lib/cartopy/tests/test_crs.py
res = target_proj.transform_points(x=rlons, y=rlats,
src_crs=src_proj)
unrotated_lon, unrotated_lat, unrotated_dat = res.transpose()

This comment has been minimized.

@pelson

pelson Feb 12, 2013

Member

and here.

@pelson

pelson Feb 12, 2013

Member

and here.

Show outdated Hide outdated lib/cartopy/_crs.pyx
* y - the y coordinates (array), in ``src_crs`` coordinates, to transform
* z - (optional) the z coordinates (array), in ``src_crs`` coordinates, to transform
* x - the x coordinates (array), in ``src_crs`` coordinates,
to transform. May be 1 or 2 dimensions.

This comment has been minimized.

@pelson

pelson Feb 12, 2013

Member

I would normally say: May be 1 or 2 dimensional. or alternatively May have 1 or 2 dimensions.

@pelson

pelson Feb 12, 2013

Member

I would normally say: May be 1 or 2 dimensional. or alternatively May have 1 or 2 dimensions.

Show outdated Hide outdated lib/cartopy/_crs.pyx
* y - the y coordinates (array), in ``src_crs`` coordinates,
to transform
* z - (optional) the z coordinates (array), in ``src_crs``
coordinates, to transform. Not applicable for projections.

This comment has been minimized.

@pelson

pelson Feb 12, 2013

Member

I'd remove the final sentence.

This CRS has no concept of a "Projection" - it is a subclass of CRS and therefore should not know about it.

@pelson

pelson Feb 12, 2013

Member

I'd remove the final sentence.

This CRS has no concept of a "Projection" - it is a subclass of CRS and therefore should not know about it.

@cpelley

This comment has been minimized.

Show comment
Hide comment
@cpelley

cpelley Feb 12, 2013

Contributor

Thanks @pelson for your comments, Iv updated the PR with your suggested changes

Contributor

cpelley commented Feb 12, 2013

Thanks @pelson for your comments, Iv updated the PR with your suggested changes

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Feb 12, 2013

Member

Thanks @cpelley. All looks good. Thanks for this!

Member

pelson commented Feb 12, 2013

Thanks @cpelley. All looks good. Thanks for this!

pelson added a commit that referenced this pull request Feb 12, 2013

Merge pull request #192 from cpelley/trans_ndim_pnts
Added support for transforming multi-dimensional arrays with ``CRS.transform_points``.

@pelson pelson merged commit e68fae4 into SciTools:master Feb 12, 2013

1 check passed

default The Travis build passed
Details

@cpelley cpelley deleted the cpelley:trans_ndim_pnts branch Feb 12, 2013

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