Repository navigation
Make frame attribute equivalency checks symmetric - #10658
Conversation
pllim
added
Affects-dev
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Thanks very much! This looks good - I particularly appreciate the great tests - just some very minor and rather nitpicky comments. Hardly worth making another commit, but, hey, you can then squash the two commits and add this PR number to the old changelog entry... But am approving since if you prefer to be just done, that is fine too!
| if left_fattr is right_fattr: | ||
| # shortcut if it's exactly the same object | ||
| return True | ||
| if left_fattr is None or right_fattr is None: |
There was a problem hiding this comment.
I was about to write "what if both are None?" when I realized that case is caught by the clause above. How about making this "elif" to make that link clearer?
| if isinstance(right_fattr, r.BaseRepresentationOrDifferential): | ||
| left_is_repr = isinstance(left_fattr, r.BaseRepresentationOrDifferential) | ||
| right_is_repr = isinstance(right_fattr, r.BaseRepresentationOrDifferential) | ||
| if left_is_repr or right_is_repr: |
There was a problem hiding this comment.
How about reversing the order a bit:
if left_is_repr and right_is_repr:
<actual useful code>
elif left_is_repr or right_is_repr:
return False
There was a problem hiding this comment.
I like this a lot. I consider indentation harmful - especially a lone else: floating in the middle of nowhere.
I picked the original order to have the smallest possible diff to make the review easier, so now I'll make it better.
| if left_is_coord and right_is_coord: | ||
| # both are coordinates | ||
| if left_fattr.is_equivalent_frame(right_fattr): | ||
| return np.all(left_fattr == right_fattr) |
There was a problem hiding this comment.
Actually, thinking about how this continues, since we want to make sure the code always returns True or False, I think it would make sense to guard the code at the end in a try/except, as in
try:
return np.all(left_attr == right_attr)
except Exception:
return False
With that, in principle, you could have only the both repr and both frame checks, since that would catch all mismatches. But code is probably clearer as you have it!
There was a problem hiding this comment.
... and catching Exception might hide broken stuff. I have a slight preference for the status quo due to that.
|
I appreciate nitpicking :-) On the changelog entry: is it needed since it is essentially a development version bugfix, and if so, what under which version should I put it? |
|
@ludwigschwardt - I would just add the PR number to whatever was added before, since this extends that addition (i.e., no new line of text needed). (Note that I didn't check what was added before, so feel free to tell me this makes no sense!) |
The `BaseCoordinateFrame._frameattr_equiv(left_fattr, right_fattr)` check is not symmetric with respect to the two attributes. If the left attribute is a coordinate frame or representation and the right one isn't, the method returns False. If the right attribute is a coordinate frame or representation and the left one isn't, one is at the mercy of the underlying equality operator instead. This typically results in a TypeError (or a FutureWarning in the case of an EarthLocationAttribute vs an unspecified None). This was encountered when calculating the angular separation between a star and the Sun in a common AltAz frame. The Sun SkyCoord had an extra `obsgeoloc` frame attribute due to its GCRS origins while the star SkyCoord had this as None. Then `Sun.separation(star)` works but `star.separation(Sun)` raises a TypeError.
3fea35f to
e667d31
Compare
|
#10290 has no changelog entry, so that's that :-) I've squashed the suggested changes together. |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Thanks, looks great now!
If you have the energy to add tests for the missing coverage, then that would be great, but don't feel obliged (it was missing before -- if it had been better you wouldn't have had a PR to make...)
|
I considered that. One problem is that it is quite hard to get a misbehaving frame attribute in normal use. The only common type mismatch is when you have |
|
OK, no worries - as you didn't make the coverage any worse, let's just merge! |
We should be able to remove this once Astropy 4.1 is released, which includes astropy/astropy#10658.
…attr-checks Make frame attribute equivalency checks symmetric
…ecks Make frame attribute equivalency checks symmetric
Astropy 4.1 causes some Solar System objects to shift by 5 mas in the body unit tests. Since the tolerance is 1 mas, this causes the tests to fail. This is most likely due to astropy/astropy#10666, which improves the treatment of solar light deflection for nearby objects. Take the new positions as gospel, since the old behaviour is considered to be a bug. Now that Astropy 4.1 is out we can also remove a workaround for 4.1rc1 (fixed in astropy/astropy#10658).
3 participants
Description
The
BaseCoordinateFrame._frameattr_equiv(left_fattr, right_fattr)check is not symmetric with respect to the two attributes.If the left attribute is a coordinate frame or representation and the right one isn't, the method returns
False.If the right attribute is a coordinate frame or representation and the left one isn't, one is at the mercy of the underlying equality operator instead. This typically results in a
TypeError(or aFutureWarningin the case of anEarthLocationAttributevs an unspecifiedNone).This was encountered when calculating the angular separation between a star and the Sun in a common AltAz frame. The Sun
SkyCoordhad an extraobsgeolocframe attribute due to its GCRS origins while the starSkyCoordhad this asNone. Thensun.separation(star)works butstar.separation(sun)raises aTypeError.`
This extends @ayshih's changes in #10290, so it can be considered a bug fix of the development version.