Coordinates allow initialisation without copies - #4883

p.s. If it were up to me, I would not change the units for repr either, i.e., I would remove all use of in_frame_units, but I have left that as is for now.

@mhvk Do you know if the failure in astropy.nddata is related to this PR?

@MSeifert04 - yes, darn, it is the second commit that breaks some things in nddata by not implicitly converting attributes to frame-specific units. It may be that some quantities are not treated correctly; will need to investigate.

(more generally, I think that code should just return whatever the user put in, not think it "knows better").

I need to look back in the original PR(s), but there was some discussion of this and (in the end) agreement that having a uniform and predictable representation was good. I vaguely recall some more compelling arguments, but I can't remember off the top of my head. In any case changing this behavior now would probably break a code out there in the wild. I'm sure there is plenty of code out there that relies on the repr being in degrees and then blindly applies .value. Not the way it's supposed to work, but I bet people do it.

@MSeifert04 - the nddata was indeed related: a wcs was initialized assuming ra was in degrees. I pushed a correction.

@taldcroft - I don't mind about repr (even if I disagree), but it beats me why one would change input on the user.

Anyway, it sounds like I better split this PR into two, as I presume my no-copy changes to representation are less controversial...

@mhvk Thank you for checking. I feared something got broken while merging my PRs (... again). It's a relief to know you broke it 😄

it beats me why one would change input on the user.

@mhvk - one reason is that there may not be a right answer as to the "format" of the input. The current design puts a premium on flexibility of the inputs:

In [8]: c = SkyCoord([1*u.deg, '2h3m2s'], [2*u.deg, 0.2*u.radian])

In [9]: c
Out[9]: 
<SkyCoord (ICRS): (ra, dec) in deg
    [(1.0, 2.0), (30.75833333, 11.4591559)]>

In pre-historic times I argued that SkyCoord should have a format just like time, but I lost that one. So basically now it needs to have a built-in default format.

Yes, I remember that discussion....

Note that I don't mind repr giving a uniform format -- I just mind coord.ra not keeping what the user passed in (at least if it is possible to keep it, i.e., it is a float with a unit to start with); it is a waste of memory, and forces users who have large arrays of coordinates through hoops to avoid needless duplication of data.

@mhvk - I see your point. You probably know that internally this is actually the case:

In [11]: c = SkyCoord(1*u.hr, 2*u.deg)

In [12]: c.data._lon
Out[12]: <Longitude 1.0 hourangle>

But you can't get at that via the public API (at least to my recollection). Note however that it would seem to be a little awkward if the ra repr (which you agree should be uniform) somehow diverged from the value, for instance:

In [15]: c.ra
Out[15]: <Longitude 14.999999999999998 deg>

In [16]: c.ra.value
1.0  # If c.ra was actually in hr but the repr was in deg

I now looked further and this convention certainly makes it very difficult to implement reshape and so on without always making copies. How annoying.

OK, since what I really need is to able to instantiate SkyCoord without copying the underlying data, I worked through the whole chain and saw I could do that without touching what c.ra does. So, this PR now just avoids needless copies in the representations, and adds new copy options to SkyCoord and BaseFrame which allow one to instantiate without copying (but by default copies are made, as before).

Arrgghh -- see some leftover from the previous commits... Will update shortly.

Tests pass locally. Should now be all OK.

@mhvk - OK, I think I'm willing to take a chance with this. I have some misgivings that there might be some hidden side-effects of not copying the frames that I've missed, but I agree with you that this is practically speaking more realistic.

Definitely need to add an entry in the changelog about this, though ("API changes", maybe?), just so we can keep track in case someone has some weird copy-related issue that comes from this. (And also the inor thing I entioned above).

Feel free to add those, (with ci skip), and then I'll merge this.

Your comment made me realize I should not be lazy, and just mention the copy option in all the built-in frames... So, that is now done. I decided not to skip ci since I might get the sphinx build wrong...

The appveyor build is obviously unrelated, and since the only thing I was testing here is the sphinx run, plus @eteq was ready to merge pending the addition of documentation, I'll merge. Surely it is still Friday somewhere...

@mhvk - indeed - still 8 more hours until it's friday nowhere, and we have an intercontinental dev team ;)

Meant to merge this but I mis-read your penultimate comment and thought you were still going to do that. So 👍 here!