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

Fix improper application of deg2rad/rad2deg in _crs.pyx #625

Merged
merged 4 commits into from Jun 24, 2015

Conversation

Projects
None yet
5 participants
@Jeitan
Contributor

Jeitan commented Jun 12, 2015

Modified transform_points in _crs.pyx to only apply rad2deg/deg2rad on the first 2 elements of the result (the lat and lon, not the height in meters). Added a test to test_crs.py to verify the result.

As far as I can tell, the test I added passes, although one other test in test_crs.py fails (looks like a significant digit issue in the test).

Fixes issue #623 .

Please review to make sure everything looks good, this is my first attempt at contributing!

change to _crs.pyx
Only apply rad2deg/deg2rad on the first 2 elements of the result (the
lat and lon, not the height in meters).  Added a test to test_crs.py to
verify.

@QuLogic QuLogic added the Pending CLA label Jun 12, 2015

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Jun 12, 2015

Member

Thanks for the contribution, @Jeitan. If I'm not mistaken, you will need to fill out the CLA first.

Member

QuLogic commented Jun 12, 2015

Thanks for the contribution, @Jeitan. If I'm not mistaken, you will need to fill out the CLA first.

Show outdated Hide outdated lib/cartopy/_crs.pyx
@Jeitan

This comment has been minimized.

Show comment
Hide comment
@Jeitan

Jeitan Jun 12, 2015

Contributor

@QuLogic Thanks for that info, I will take a look.

Contributor

Jeitan commented Jun 12, 2015

@QuLogic Thanks for that info, I will take a look.

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Jun 12, 2015

Member

Thanks for the contribution, @Jeitan. If I'm not mistaken, you will need to fill out the CLA first.

👍 @QuLogic

Please review to make sure everything looks good, this is my first attempt at contributing!

This looks awesome! Thanks @Jeitan.

Member

pelson commented Jun 12, 2015

Thanks for the contribution, @Jeitan. If I'm not mistaken, you will need to fill out the CLA first.

👍 @QuLogic

Please review to make sure everything looks good, this is my first attempt at contributing!

This looks awesome! Thanks @Jeitan.

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Jun 12, 2015

Member

There is a lot of noise on the travis tests at present. I'm keen to solve that, but in the meantime, I wouldn't hold this up for it.

In terms of the tests I can clearly see that are failing as a result of this:

test_pep8_conformance (cartopy.tests.test_coding_standards.TestCodeFormat) ... /home/travis/cartopy/tests/test_crs.py:149:1: W293 blank line contains whitespace
cartopy/tests/test_crs.py:155:1: W293 blank line contains whitespace
cartopy/tests/test_crs.py:158:1: W293 blank line contains whitespace
cartopy/tests/test_crs.py:162:23: E231 missing whitespace after ','
cartopy/tests/test_crs.py:163:23: E231 missing whitespace after ','
cartopy/tests/test_crs.py:164:23: E231 missing whitespace after ','
cartopy/tests/test_crs.py:165:1: W293 blank line contains whitespace
cartopy/tests/test_crs.py:170:1: W293 blank line contains whitespace

There are two numerical test failiures that I'm confident aren't as a result of this change:

======================================================================

FAIL: cartopy.tests.crs.test_robinson.test_transform_point

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/nose/case.py", line 197, in runTest

self.test(*self.arg)

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/Cartopy-0.13.dev0-py2.7-linux-x86_64.egg/cartopy/tests/crs/test_robinson.py", line 46, in test_transform_point

assert_arr_almost_eq(result, (2376187.15910577, 7275318.94714286))

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/numpy/testing/utils.py", line 811, in assert_array_almost_equal

header=('Arrays are not almost equal to %d decimals' % decimal))

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/numpy/testing/utils.py", line 644, in assert_array_compare

raise AssertionError(msg)

AssertionError:

Arrays are not almost equal to 6 decimals

(mismatch 100.0%)

x: array([ 2376187.27182751, 7275317.81573085])

y: array([ 2376187.15910577, 7275318.94714286])

======================================================================

FAIL: cartopy.tests.crs.test_robinson.test_transform_points

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/nose/case.py", line 197, in runTest

self.test(*self.arg)

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/Cartopy-0.13.dev0-py2.7-linux-x86_64.egg/cartopy/tests/crs/test_robinson.py", line 63, in test_transform_points

[[2376187.15910577, 7275318.94714286, 0]])

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/numpy/testing/utils.py", line 811, in assert_array_almost_equal

header=('Arrays are not almost equal to %d decimals' % decimal))

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/numpy/testing/utils.py", line 644, in assert_array_compare

raise AssertionError(msg)

AssertionError:

Arrays are not almost equal to 6 decimals

(mismatch 66.6666666667%)

x: array([[ 2376187.27182751, 7275317.81573085, 0. ]])

y: array([[ 2376187.15910577, 7275318.94714286, 0. ]])
Member

pelson commented Jun 12, 2015

There is a lot of noise on the travis tests at present. I'm keen to solve that, but in the meantime, I wouldn't hold this up for it.

In terms of the tests I can clearly see that are failing as a result of this:

test_pep8_conformance (cartopy.tests.test_coding_standards.TestCodeFormat) ... /home/travis/cartopy/tests/test_crs.py:149:1: W293 blank line contains whitespace
cartopy/tests/test_crs.py:155:1: W293 blank line contains whitespace
cartopy/tests/test_crs.py:158:1: W293 blank line contains whitespace
cartopy/tests/test_crs.py:162:23: E231 missing whitespace after ','
cartopy/tests/test_crs.py:163:23: E231 missing whitespace after ','
cartopy/tests/test_crs.py:164:23: E231 missing whitespace after ','
cartopy/tests/test_crs.py:165:1: W293 blank line contains whitespace
cartopy/tests/test_crs.py:170:1: W293 blank line contains whitespace

There are two numerical test failiures that I'm confident aren't as a result of this change:

======================================================================

FAIL: cartopy.tests.crs.test_robinson.test_transform_point

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/nose/case.py", line 197, in runTest

self.test(*self.arg)

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/Cartopy-0.13.dev0-py2.7-linux-x86_64.egg/cartopy/tests/crs/test_robinson.py", line 46, in test_transform_point

assert_arr_almost_eq(result, (2376187.15910577, 7275318.94714286))

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/numpy/testing/utils.py", line 811, in assert_array_almost_equal

header=('Arrays are not almost equal to %d decimals' % decimal))

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/numpy/testing/utils.py", line 644, in assert_array_compare

raise AssertionError(msg)

AssertionError:

Arrays are not almost equal to 6 decimals

(mismatch 100.0%)

x: array([ 2376187.27182751, 7275317.81573085])

y: array([ 2376187.15910577, 7275318.94714286])

======================================================================

FAIL: cartopy.tests.crs.test_robinson.test_transform_points

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/nose/case.py", line 197, in runTest

self.test(*self.arg)

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/Cartopy-0.13.dev0-py2.7-linux-x86_64.egg/cartopy/tests/crs/test_robinson.py", line 63, in test_transform_points

[[2376187.15910577, 7275318.94714286, 0]])

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/numpy/testing/utils.py", line 811, in assert_array_almost_equal

header=('Arrays are not almost equal to %d decimals' % decimal))

File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/numpy/testing/utils.py", line 644, in assert_array_compare

raise AssertionError(msg)

AssertionError:

Arrays are not almost equal to 6 decimals

(mismatch 66.6666666667%)

x: array([[ 2376187.27182751, 7275317.81573085, 0. ]])

y: array([[ 2376187.15910577, 7275318.94714286, 0. ]])
Fix pep8 conformity
Removed spaces in blank lines and add space after commas.
@Jeitan

This comment has been minimized.

Show comment
Hide comment
@Jeitan

Jeitan Jun 12, 2015

Contributor

@pelson Thanks! I made the spaces fixes...

@QuLogic was something like this what you had in mind? It's slightly more code, but yes less copying.

Not committed:

    result = np.empty([npts, 3], dtype=np.double)
    if src_crs.is_geodetic():
        result[:, 0] = np.deg2rad(x)
        result[:, 1] = np.deg2rad(y)
    else:
        result[:, 0] = x
        result[:, 1] = y
    # if a z has been given, put it in the result array which will be
    # transformed in-place
    if z is None:
        result[:, 2] = 0
    else:
        result[:, 2] = z

    # call proj.4. The result array is modified in place.
    status = pj_transform(src_crs.proj4, self.proj4, npts, 3,
                          &result[0, 0], &result[0, 1], &result[0, 2])

I sent the CLA, thanks for the requirement info!

Contributor

Jeitan commented Jun 12, 2015

@pelson Thanks! I made the spaces fixes...

@QuLogic was something like this what you had in mind? It's slightly more code, but yes less copying.

Not committed:

    result = np.empty([npts, 3], dtype=np.double)
    if src_crs.is_geodetic():
        result[:, 0] = np.deg2rad(x)
        result[:, 1] = np.deg2rad(y)
    else:
        result[:, 0] = x
        result[:, 1] = y
    # if a z has been given, put it in the result array which will be
    # transformed in-place
    if z is None:
        result[:, 2] = 0
    else:
        result[:, 2] = z

    # call proj.4. The result array is modified in place.
    status = pj_transform(src_crs.proj4, self.proj4, npts, 3,
                          &result[0, 0], &result[0, 1], &result[0, 2])

I sent the CLA, thanks for the requirement info!

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Jun 12, 2015

Member

Yep, that's what I meant.

Member

QuLogic commented Jun 12, 2015

Yep, that's what I meant.

Moved deg2rad call
Upon suggestion, moved if statement and deg2rad to prevent copying
@Jeitan

This comment has been minimized.

Show comment
Hide comment
@Jeitan

Jeitan Jun 12, 2015

Contributor

Well, it's probably not a huge deal, but it's also not hard, so I just went ahead and made the change.

Contributor

Jeitan commented Jun 12, 2015

Well, it's probably not a huge deal, but it's also not hard, so I just went ahead and made the change.

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Jun 22, 2015

Member

Thanks @Jeitan - would you be able to send over the CLA, and also rebase - the tests should now be working as expected.

Thanks!

Member

pelson commented Jun 22, 2015

Thanks @Jeitan - would you be able to send over the CLA, and also rebase - the tests should now be working as expected.

Thanks!

@Jeitan

This comment has been minimized.

Show comment
Hide comment
@Jeitan

Jeitan Jun 23, 2015

Contributor

@pelson Great, thanks. I merged in the Scitools master. And the CLA, I thought I emailed it to the correct place, but I guess not. Where should I send it?

Contributor

Jeitan commented Jun 23, 2015

@pelson Great, thanks. I merged in the Scitools master. And the CLA, I thought I emailed it to the correct place, but I guess not. Where should I send it?

@pelson pelson removed the Pending CLA label Jun 24, 2015

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Jun 24, 2015

Member

Thanks @Jeitan. Congratulations on your first commit! 👍

Member

pelson commented Jun 24, 2015

Thanks @Jeitan. Congratulations on your first commit! 👍

pelson added a commit that referenced this pull request Jun 24, 2015

Merge pull request #625 from Jeitan/no_degrad_on_meters
Fix improper application of deg2rad/rad2deg in _crs.pyx

@pelson pelson merged commit 44e5ae8 into SciTools:master Jun 24, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@marqh

This comment has been minimized.

Show comment
Hide comment
@marqh

marqh Jun 24, 2015

Member

I merged in the Scitools master. And the CLA, I thought I emailed it to the correct place, but I guess not. Where should I send it?

it was the correct place, you should have a signed copy winging its way back to you now

Member

marqh commented Jun 24, 2015

I merged in the Scitools master. And the CLA, I thought I emailed it to the correct place, but I guess not. Where should I send it?

it was the correct place, you should have a signed copy winging its way back to you now

@pelson pelson added this to the 0.13 milestone Jun 24, 2015

@Jeitan

This comment has been minimized.

Show comment
Hide comment
@Jeitan

Jeitan Jun 24, 2015

Contributor

Great! Thanks for making the process easy, all :)

Contributor

Jeitan commented Jun 24, 2015

Great! Thanks for making the process easy, all :)

@Jeitan Jeitan deleted the Jeitan:no_degrad_on_meters branch Jun 24, 2015

@QuLogic QuLogic modified the milestones: 0.13.0, 0.15 Jul 4, 2015

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 27, 2017

Coverage Status

Changes Unknown when pulling 5e7794e on Jeitan:no_degrad_on_meters into ** on SciTools:master**.

coveralls commented Feb 27, 2017

Coverage Status

Changes Unknown when pulling 5e7794e on Jeitan:no_degrad_on_meters into ** on SciTools:master**.

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