Distance setting twice bug #793
Conversation
|
The travis error is the still-errororing numpy 1.4.x tests - all the rest succeeded, so this should be good to merge. |
|
This looks good to me, and I don't feel strongly about 0.2.0 vs 0.2.1/0.3, so I'll leave it up to @iguananaut. But in any case, I think we shouldn't delay the release because of this, so if it comes to it, @iguananaut could simply cherry-pick the first two commits, and @eteq could then later rebase this PR if needed. |
|
I think that it might be worth including the bug fix in 0.2.0 and thinking more about the change of |
|
(also, the docs would need to be updated to show the support for Quantity objects, etc. - I think it would just be cleaner to apply the bug fix now and do the full switch to Quantity in a separate PR) |
|
Note that I would actually consider all but the last commit to be "bug fix" - that is, the middle commits actually allow setting distance with a Quantity, but don't convert a distance into a Quantity. I'll split that out now as a separate PR, then. |
|
@eteq - ah I see. Splitting out the last commit sounds good, as that is the only one that I'm unsure about at this time (I'm definitely in favor of having |
|
It's not immediately apparent in the thread here, but I just pushed up a change that removed the final commit (that changed So this should now be uncontroversial. (Also, the build failure is still unrelated to this PR) |
|
Looks good to me! |
|
I'll leave it to @iguananaut to finalize the merge and backport, then. |
|
Um...this seems to have caused some bug to fail (or maybe it was already failing?) in the timing prediction tests. That's not in 0.2 so it's not a problem for now but I thought I'd point that out: https://travis-ci.org/astropy/astropy/jobs/4911115 |
|
@iguananaut - this will likely be fixed with #766 |
|
yeah, those failures will happen randomly until #766 is merged - see https://travis-ci.org/eteq/astropy/builds/4891789 , which should be the same commit as the one that failed... but it succeeds. |
Distance setting twice bug
3 participants
This fixes a couple of bugs I just noticed relating to the
coordinatesDistanceclass and its interaction with coordinate classes.The first two commits deal with a genuine bug - the cached cartesian representation of the coordinate was not properly being cleared, so setting the distance twice lead to the second distance not being updated. This is a pretty trivial fix, and I added a regression test for it.
The last 3 commits are arguable "features", although I consider them bugs in that they were supposed to be done and were forgotten about (and are also pretty trivial). They deal with integrating
Quantitywith theDistanceclass. Previously, you could not dosomecoordinate.distance = 2 * u.kpcbecauseDistancewasn't set up to acceptQuantity, and this fixes that. This also changesDistanceto be a subclass ofQuantity. That was actually the intended behavior, but it was forgotten about until just now.I realize we're right against the wire for 0.2, but I don't see any reason why we wouldn't include this. The first two commits certainly should go in, as it's unambiguously a bug. If anyone has complaints about the others, I can just move them to a separate PR for 0.2.1/0./3, but as I said before, I think they're actually bugs in that this just adjusts to the originally intended behavior.