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

Make output of to_geodetic a namedtuple #6237

Merged
merged 4 commits into from
Jun 18, 2017
Merged

Conversation

eteq
Copy link
Member

@eteq eteq commented Jun 18, 2017

This is a very straightforward PR that does just what it says: makes EarthLocation.to_geodetic output a namedtuple instead of a regular tuple, allowing things like:

earthlocation.to_geodetic().lon

which seems a lot more readable than:

earthlocation.to_geodetic()[0]

While working on #5752 and #6226 et al I realized this would make some things a lot easier...

@eteq eteq added this to the v2.0.0 milestone Jun 18, 2017
@eteq eteq requested a review from mhvk June 18, 2017 05:43
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

I've been meaning to change these things to structured arrays at some point, but have been stopped by the need to have different units. This makes that path forward slightly harder (would need to be based on a recarray, I suppose), but, heck, this is definitely an improvement! So, 👍 from me.

Only question: the class itself has full names as the attributes (longitude, latitude, height); it is perhaps slightly confusing to use short names here. On the other hand, in hindsight, I think EarthLocation should have had short names -- even the initializer uses them! So, probably more a reason to think of eventually deprecating the long names...

@mhvk
Copy link
Contributor

mhvk commented Jun 18, 2017

Appveyor had a very pecular failure - so I restarted it.

@eteq
Copy link
Member Author

eteq commented Jun 18, 2017

@mhvk - good point on lat/lon on the main object! The commit I pushed up adds them as properties and also deprecates latitude/longitude.

(If you think that's too agressive, can skip the deprecation for now, but I think adding lat and lon as options is a good idea to make it all internally consistent.)

@mhvk
Copy link
Contributor

mhvk commented Jun 18, 2017

I think it is good to deprecate the long names, and a LTS is a good time to do it!

@mhvk
Copy link
Contributor

mhvk commented Jun 18, 2017

But we do need to mention the deprecation in CHANGES.rst -- which needs a rebase anyway. Otherwise, ready to go as far as I'm concerned (I'm assuming the decrease in coverage is a fluke).

@bsipocz
Copy link
Member

bsipocz commented Jun 18, 2017

@mhvk - the coverage decrease must be unrelated as commits before that passed it.

@mhvk
Copy link
Contributor

mhvk commented Jun 18, 2017

It is probably that the now deprecated properties do not get used at all -- anyway, not important.

@bsipocz
Copy link
Member

bsipocz commented Jun 18, 2017

My guess would be that master changed since, and coveralls seems to compare the coverage of the given PR to the latest run it had. So after a rebase/merging, this decrease should go away.

@eteq
Copy link
Member Author

eteq commented Jun 18, 2017

Updated the CHANGES.rst appropriately. The tests already passed in https://travis-ci.org/astropy/astropy/builds/244269953 so I think this is all set - merging so as not to have to worry about future changelog rebases.

@eteq eteq merged commit cdfcfa9 into astropy:master Jun 18, 2017
@eteq eteq deleted the earth-namedtuple branch June 18, 2017 22:15
@pllim
Copy link
Member

pllim commented Jun 21, 2017

I am very surprised that tests passed. When I run code from one of the tests still using deprecation longtitude in IPython, I see AstropyDeprecationWarning!

@pllim
Copy link
Member

pllim commented Jun 21, 2017

Oh, wait... Astropy's conftest.py does not turn AstropyDeprecationWarning into exceptions! Was that intentional?!

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

Successfully merging this pull request may close these issues.

None yet

4 participants