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

fix for first item in 4210 by doing labmda->lamb #4211

Merged
merged 5 commits into from
Oct 5, 2015

Conversation

eteq
Copy link
Member

@eteq eteq commented Oct 2, 2015

This fixes the issue pointed out in #4210 by simply renaming lambda -> lamb (the issue was that lambda is of course a reserved keyword in python.

@eteq eteq added this to the v1.1.0 milestone Oct 2, 2015
@eteq eteq added the Affects-dev PRs and issues that do not impact an existing Astropy release label Oct 2, 2015
@astrofrog
Copy link
Member

@eteq - I wonder if lam would be a more 'common' abbreviation? Otherwise I'm thinking of the animal :-/

@mhvk
Copy link
Contributor

mhvk commented Oct 2, 2015

The PEP8 way to deal with name conflicts with reserved keywords is to append an underscore, i.e., lambda_; from http://legacy.python.org/dev/peps/pep-0008/
"""
If your public attribute name collides with a reserved keyword, append a single trailing underscore to your attribute name. This is preferable to an abbreviation or corrupted spelling. (However, notwithstanding this rule, 'cls' is the preferred spelling for any variable or argument which is known to be a class, especially the first argument to a class method.)
"""

Though, personally, I'd prefer just l, b, as I never think of these as greek letters. Also, @barentsen mentions that is already used for BarycentricTrueEcliptic; shouldn't the two be consistent?!

@mhvk mhvk changed the title fix for first item in 4120 by doing labmda->lamb fix for first item in 4210 by doing labmda->lamb Oct 2, 2015
@mhvk
Copy link
Contributor

mhvk commented Oct 2, 2015

p.s. Fix is for #4210 (not 4120); I went ahead and edited title and first entry.

@embray
Copy link
Member

embray commented Oct 2, 2015

Incidentally, astropy.modeling.projections already uses "lam" as short for "lambda". I'm not saying it's better or worse than "lamb" (I think I might like "lamb" better because it's cute 🐑 :) but maybe we should at least have a consistent way of abbreviating lambda: https://github.com/astropy/astropy/blob/master/astropy/modeling/projections.py#L721

I'd also be fine with "lambda_" per @mhvk's comment above, and would be willing to change the projection models to match it. Anyways I don't have a strong feeling about which way we go as long as we try to keep some inter-subpackage consistency.

@astrofrog
Copy link
Member

I'd vote for lam over lambda_, and I think lam is a pretty common abbreviation, e.g. F_lam

@embray
Copy link
Member

embray commented Oct 2, 2015

Can also always provide the unicode spelling as an alias :)

@barentsen
Copy link
Contributor

In plain English I would always talk about ecliptic longitude and latitude, hence lon and lat is the first thing I intuitively tried when playing with the ecliptic coordinates. Would this be an option? (In this logic also galactic should have lat and lon arguments however.)

@mhvk
Copy link
Contributor

mhvk commented Oct 2, 2015

Since not just BarycentricTrueEcliptic but also galactic uses l and b, it would seem most consistent to use l and b here too.

@eteq
Copy link
Member Author

eteq commented Oct 2, 2015

@mhvk @barentsen @astrofrog - just to clarify, I got these names because that's what I understand to be the convention among people who use ecliptic coordinates. That is, l/b/r (or maybe l/b/d) is used for heliocentric ecliptic coordinates, and lambda/beta/delta are used for geocentric ecliptic. My sources for that are https://en.wikipedia.org/wiki/Ecliptic_coordinate_system and an old textbook I found in the Yale astronomy library (which I think has since been shredded...)

@eteq
Copy link
Member Author

eteq commented Oct 2, 2015

But I strongly favor lam over lambda_. I know about the convention, @mhvk, but I think most users will not, so they'll be more confused by it than an abbreviation. I'd certainly use that if it was an internal variable, but for something public like this, I prefer lam.

All that said, it could be that we shouldn't care about the convention at all and just do l/b/distance for all of them. Or even lat/lon/distance, as @barentsen suggested. I don't care that much, but was just trying to do whatever seems to be most consistent with what people used to working with ecliptic coordinates might expect.

@eteq
Copy link
Member Author

eteq commented Oct 2, 2015

Oh, and FYI, @barentsen, you can always do coord.spherical.lon or coord.spherical.lat. That's always available for any coordinate system - the conventional names are just there for what people are "used to".

@mhvk
Copy link
Contributor

mhvk commented Oct 2, 2015

@eteq - OK, fair enough, I should have realised you had thought this through! Overall, I'd prefer to go with lambda_, beta, delta, then, following PEP8.

p.s. Note that the Nautical Almanac Office in planetary and lunar coordinates -- https://books.google.ca/books?id=2g_TICVzdSAC&pg=PR9&lpg=PR9&dq=geocentric+ecliptic+coordinates&source=bl&ots=CfyVxOk3Yq&sig=Jkx-UbBHXFu--dtubJqITbT72LM&hl=en&sa=X&ved=0CCIQ6AEwATgKahUKEwjm4vuTyqTIAhUCdj4KHdRXDBM#v=onepage&q=geocentric%20ecliptic%20coordinates&f=false -- seems to use rho for the distance, which seems a little more usual for a radial distance (but the astronomical almanac uses pi for the Moon; https://books.google.ca/books?id=ioogcZf033wC&pg=PA344&lpg=PA344&dq=geocentric+ecliptic+coordinates&source=bl&ots=0GOzanXvsY&sig=YILpG3z8QifgwCkwdfX1Q0Wb960&hl=en&sa=X&ved=0CC8Q6AEwBDgUahUKEwjayPnwyqTIAhXCcD4KHWIqDMs#v=onepage&q=geocentric%20ecliptic%20coordinates&f=false).

@mhvk
Copy link
Contributor

mhvk commented Oct 2, 2015

OK, messages crossed. I also don't care all that much, but have a slight preference for just doing l, b, distance over adding abbreviations. But your call.

@eteq
Copy link
Member Author

eteq commented Oct 2, 2015

Honestly, what we really need is someone who actually knows the conventions people actually use... But awkwardly, it seems we can't get their feeback until we implement it... :/

At any rate, I tried to do a few more searches and concluded it's just a mess - lots of different conventions for what letter to use... In addition to what @mhvk dug up I found some refernces to lambda/phi/delta.

So I guess now I tend towards l/b/distance just to make us internally consistent, or just changing all the ecliptic systems to lat/lon/distance. Any strong opinions between those? (If no I'll just make a command decision)

@mhvk
Copy link
Contributor

mhvk commented Oct 2, 2015

weak preference for l, b, by analogy with galactic. To me, longitude and latitude seem rather mixed in with locations on an object.

pinging JJ (@ijiraq) as someone who I think actually works with ecliptic systems in his Oort-cloud hunts.

@ijiraq
Copy link

ijiraq commented Oct 2, 2015

Since @mhvk pinged me.
I think I prefer l, b, distance but I'm suspect many solar system people will wonder why galactic notation is being used for Ecliptic coordinates.

As an example, pyEphem went through a period of using 'long' (oops) and then switched to lon/lat/distance

Conclusion, lon/lat might be more recognized but l/b will not be confusing. I do not like 'lam' or 'lamb' or 'lambda_'

(how many characters in an attribute name is too many? , I guess 5).

@mtbannister
Copy link

I too am happier with lon/lat/distance. Confusion with Galactic notation is a real issue with l/b.

@astrofrog
Copy link
Member

I would vote for lon/lat, since I don't think anyone will complain about that option (i.e. I doubt people will have strong opinions about that, vs lam/beta and l/b)

@eteq
Copy link
Member Author

eteq commented Oct 5, 2015

Alright, sounds good - I'll change all of the ecliptic names to lon/lat/distance for consistency.

@astrofrog
Copy link
Member

@eteq - better get in the change soon to leave Travis/Appveyor time to run before the feature freeze :)

Not really related to this PR, except that a "find all" on "ecliptic"
revealed its existence...
astrofrog added a commit that referenced this pull request Oct 5, 2015
fix for first item in 4210 by doing labmda->lamb
@astrofrog astrofrog merged commit 40e8d33 into astropy:master Oct 5, 2015
@eteq eteq deleted the fix-4120 branch October 5, 2015 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Affects-dev PRs and issues that do not impact an existing Astropy release coordinates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants