-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 poor performance in Angle.__str__ and Angle._repr_latex_ #7004
Conversation
Hi there @vermashresth 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
@astrofrog - do we need changelog for this? I would guess not, but it's your call. |
I think we should add a changelog entry for 2.0.4 - I also think it would make sense to make sure we have a test for this to make sure this works correctly (can't check this right now) |
Don't know that I see the point in a change-log entry: all this does is speed up what was there. There is also a test of sorts already: https://github.com/astropy/astropy/blob/master/astropy/coordinates/tests/test_angles.py#L867 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me.
But I agree with @astrofrog that the __str__
method needs a test - according to coveralls
(https://coveralls.io/builds/14723125/source?filename=astropy%2Fcoordinates%2Fangles.py#L433), the new else case of __str__
is not getting hit in the test suite. I just checked it myself from @astrofrog's example in #6995, and it works fine, but that should be in the test suite too. @vermashresth, do you think you could do this soon? It would be nice to get this into Astropy 3.0 ... You might just add this to the test @mhvk mentioned in #7004 (comment) but you could also add a separate test that checks a bit more that the string makes sense...
As for the changelog, I think it might just be worth noting this in the "other changes" section. But I figured it'd be easier to just add this myself by pushing a commit to this branch, so that's what I did. You might take a quick look at 4e4567c, @vermashresth, just so you see what I did.
@eteq Sure, I can do this soon. Would the test cases for |
@vermashresth - yes, the test should be (very) similar. |
strarrangle = arrangle.__str__() | ||
|
||
assert strarrangle == '[10d14m04.2s -20d00m00s]' | ||
assert strscangle in strarrangle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but can you just test stsscangle
directly? Maybe put it just below were it is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhvk Okay I will do, any other cases that should be checked?
@vermashresth - this looks good to me. I made another commit to remove spaces (that github editor is darn annoying!). Now can be merged if tests pass. Thanks! |
Tests passed, so I'm merging (the failing doc build is unrelated and is being dealt with separately). |
Of course this already got merged, but it looked good, so I'm happy. Thanks @vermashresth ! |
Fix for poor performance in Angle.__str__ and Angle._repr_latex_
Changed the call to
to_string()
method inside__str__()
and_repr_latex_()
to be done through numpy machinery to prevent unnecessary computing and poor performance.Edit: close #6995