Let Angle subclasses decide on class of calculation results; correct Latitude __setitem__ #1851
Conversation
astropy/coordinates/angles.py
Outdated
There was a problem hiding this comment.
Do we need to go through the trouble of initing an object, just to do the bounds check? Isn't calling _validate_angle enough?
There was a problem hiding this comment.
@mdboom - I tried self.__class__._validate_angles(value) first but I got an error stating that I should pass on a Latitude class object. However, looking at it with fresh eyes, I see that this was because _validate_angles does not return anything... If I add return self at the end, it works.
Before I just go ahead, an alternative would be to give _validate_angles an optional argument of which angles to check (which would default to self). Would you prefer that approach? It does mean that if we ever decide to allow different ranges (e.g., 0 to 180), then things would more easily continue to work.
…, doing validation upfront and ensuring no temporary object is created
|
@mdboom - I think this now-simple PR is ready to go in. Could you have a final look? |
astropy/coordinates/angles.py
Outdated
There was a problem hiding this comment.
Why is checked being created here? First, the variable name sounds like a boolean to me (i.e. "were the values validated?") so I had to read the code carefully to see what's going on. But _validate_angles doesn't ever change value (or self if value is None), and instead the only action that method can take is to raise a ValueError.
So it would make more sense to me to just leave the original where _validate_angles doesn't return anything. Then you don't have to create a new variable checked, and the method name would correspond to exactly what it does.
Also, in this current form if you have an Angle object a and do a[2] = None then it ends up actually trying to set a[2] = a. Not good.
There was a problem hiding this comment.
@taldcroft - you're right, my hesitance to have a return-nothing function got the better of me here. Reverted that, which means that the corner case of None will still lead to a check of self, but will then fail in __setitem__ anyway.
There was a problem hiding this comment.
Looks better now. There is absolutely nothing wrong with return-nothing methods. Almost every class has one, it's called __init__. :-)
Let Angle subclasses decide on class of calculation results; correct Latitude __setitem__
I noticed
Angle.__array_wrap__initialises of a newAngleobject when a view would do. But correcting it, I realised this really is in the wrong place: we wantLongitudeandLatitudeto fall back toAnglewhen any calculation is done, but this may be unwanted behaviour for possible futureAnglesubclasses. Hence, this PR moves the__array_wrap__to those subclasses.I also noticed that in
Latitudeitems get set before they are being checked, which means thatlat[:]=100*u.degwill raise an error message after messing up the contents. Validation is now done first.(̶F̶o̶r̶ ̶m̶y̶ ̶o̶w̶n̶ ̶s̶a̶n̶i̶t̶y̶,̶ ̶t̶h̶i̶s̶ ̶i̶s̶ ̶b̶a̶s̶e̶d̶ ̶o̶n̶ ̶#̶1̶8̶4̶8̶,̶ ̶s̶o̶ ̶t̶h̶a̶t̶ ̶o̶n̶e̶ ̶s̶h̶o̶u̶l̶d̶ ̶b̶e̶ ̶m̶e̶r̶g̶e̶d̶ ̶f̶i̶r̶s̶t̶.̶)̶
EDIT: that was a mistake. hopeless to keep track. now split it off.