Added a `angular_radius` convenience property to `Helioprojective` #5191

Anubhav1107 · 2021-04-06T13:01:26Z

Description

Fixes #5182

pep8speaks · 2021-04-06T13:01:30Z

Hello @Anubhav1107! Thanks for updating this PR.

In the file sunpy/coordinates/frames.py:

Line 161:17: W503 line break before binary operator

In the file sunpy/coordinates/tests/test_frames.py:

Line 252:59: W504 line break after binary operator
Line 127:74: W504 line break after binary operator

Comment last updated at 2021-06-02 17:06:03 UTC

ayshih

Thanks for the PR! In addition to my comments, also remove a bunch of the empty newlines.

sunpy/coordinates/frames.py

Anubhav1107 · 2021-04-06T14:46:14Z

hey, is it ok now ?

ayshih

hey, is it ok now ?

Heh, no, it doesn't even run because you didn't import the _angular_radius() function.

This PR will also need a unit test.

sunpy/coordinates/frames.py

Anubhav1107 · 2021-04-06T15:11:48Z

hey, is it ok now ?

Heh, no, it doesn't even run because you didn't import the _angular_radius() function.

This PR will also need a unit test.

Ahh, So sorry, I totally forgot it !

dstansby · 2021-04-24T19:20:48Z

Hi @Anubhav1107, we would really like to get this into sunpy 3.0, so were wondering if you could update this in the next few days? If not, would you be okay with someone else finishing it off?

Anubhav1107 · 2021-05-12T15:24:55Z

Hey @dstansby, I am sorry to be this late, sure. I will try to finish it soon, if not I will tell you!

Anubhav1107 · 2021-05-13T12:17:52Z

hey @ayshih, this line you added
_angular_radius(self.rsun, self.observer.radius)

If I try to call this function as

time = frames.Helioprojective()
time.angular_radius

It gives
<bound method Helioprojective.angular_radius of <Helioprojective Frame (obstime=None, rsun=695700.0 km, observer=None)>>

My qsn is how do you calculate the angular_radius, If I try to insert a value in them. It gives error
as
AttributeError: 'NoneType' object has no attribute 'radius'

nabobalis · 2021-05-13T12:40:39Z

In this case, the command line is telling you that angular_radius is a "bound method" and that means you have to call the method with brackets e.g., angular_radius()

Tho it does have the "@Property" which I thought dealt with that?

EDIT:

I ran the code, the issue is that the frame you created with frames.Helioprojective() has no observer and so goes to get the radius and then errors.

I am not sure if the frames are meant to be called directly? (This is beyond my area).

But to get a working example you can do:

import astropy.units as u
from astropy.coordinates import SkyCoord
from sunpy.coordinates import frames # Not sure if we need this but maybe to register the frames?

sc = SkyCoord(0*u.deg, 0*u.deg, 5*u.km,obstime="2010/01/01T00:00:00", observer="earth", frame="helioprojective")
sc.angular_radius
<Angle 975.51961541 arcsec>

ayshih · 2021-05-15T02:43:22Z

Tho it does have the "@Property" which I thought dealt with that?

That's correct: The @property decorator must be missing to get that error. I suspect that @Anubhav1107 's local repo does not have the commits as they are in this PR, probably through not fetching commits locally after making changes through GitHub's web interface.

I am not sure if the frames are meant to be called directly? (This is beyond my area).

Using SkyCoord is recommended for most users, but frames will work correctly if called directly. Here's the equivalent without using SkyCoord:

>>> import astropy.units as u
>>> from sunpy.coordinates import Helioprojective

>>> h = Helioprojective(0*u.deg, 0*u.deg, obstime='2010-01-01', observer='earth')
>>> h.angular_radius
<Angle 975.51961541 arcsec>

from sunpy.coordinates import frames # Not sure if we need this but maybe to register the frames?

Yes, an import is needed for SkyCoord to know how to interpret frame="helioprojective", but it can just be import sunpy.coordinates.

sunpy/coordinates/frames.py

Anubhav1107 · 2021-05-16T13:21:09Z

test_ex.txt
Is this an ok unit test example or should how should I change it ?

nabobalis · 2021-05-16T13:24:24Z

test_ex.txt
Is this an ok unit test example or should how should I change it ?

It will need some work but if you can add it to the PR, we can work on it.

Anubhav1107 · 2021-05-16T13:31:52Z

sure, I tried this because when I was doing test with pytest and assert, it started giving error as
warnings._OptionError: invalid module name: 'sunpy.util.exceptions'
It maybe due to env problem so I left it out!

nabobalis · 2021-05-16T13:40:51Z

sure, I tried this because when I was doing test with pytest and assert, it started giving error as
warnings._OptionError: invalid module name: 'sunpy.util.exceptions'
It maybe due to env problem so I left it out!

Err, sounds like it could be. I am not sure what needs to be done to fix it, maybe you need to reinstall sunpy?

nabobalis · 2021-05-16T13:43:24Z

sunpy/map/frames_test.py

@@ -0,0 +1,36 @@
+import astropy.units as u


Things to do:

This code should go into https://github.com/sunpy/sunpy/blob/main/sunpy/coordinates/tests/test_frames.py not a new file

You do not need most of these imports at all and should be removed.

The test should be something like:

def test_angular_radius(): coord = frames.Helioprojective(0*u.deg,0*u.deg,5*u.km,obstime="2010/01/01T00:00:00", observer="earth") sc = SkyCoord(0*u.deg, 0*u.deg, 5*u.km,obstime="2010/01/01T00:00:00", observer="earth", frame="helioprojective") assert coord.angular_radius==sc.angular_radius

Anubhav1107 · 2021-05-16T13:58:59Z

Thanks much, I changed it. The amount of push and pull req is a little confusing

nabobalis · 2021-05-16T14:03:47Z

Thanks much, I changed it. The amount of push and pull req is a little confusing

I don't quite understand what you mean.

sunpy/coordinates/tests/test_frames.py

changelog/5191.trivial.rst

sunpy/coordinates/frames.py

ayshih · 2021-05-17T13:13:15Z

sunpy/coordinates/tests/test_frames.py

+    coord = Helioprojective(0*u.deg, 0*u.deg, 5*u.km, obstime="2010/01/01T00:00:00", observer="earth")
+    sc = SkyCoord(0*u.deg, 0*u.deg, 5*u.km, obstime="2010/01/01T00:00:00", observer="earth", frame="helioprojective")
+    assert coord.angular_radius==sc.angular_radius


This test has low value as written. sc.angular_radius simply calls Helioprojective.angular_radius, so the equality check will always succeed, as long as Heliprojective.angular_radius returns a value without an error. However, the value could be wildly wrong.

A better test would be a consistency check that coord.angular_radius is equal* to the return value of sunpy.coordinates.sun.angular_radius(coord.obstime). We have accuracy checks for the sunpy.coordinates.sun functions against The Astronomical Almanac, so that would implicitly check the accuracy as well.

* Incidentally, don't literally use ==. You should use assert_quantity_allclose().

Sure, thanks!

Hey, I am getting this output after the code

AssertionError: Not equal to tolerance rtol=1e-07, atol=0 Mismatch: 100% Max absolute difference: 2.37443473 Max relative difference: 0.00243402 x: array(973.145181) y: array(975.519615)

I can't really debug this if you don't show the code as well. The 975.519615 arcseconds is correct for this observation time. I can't guess what code is returning 973.145181 arcseconds.

Ooh,this is the code
def test_angular_radius():

coord = frames.Helioprojective(0*u.deg, 0*u.deg, 5*u.km, obstime="2010/01/01T00:00:00", observer="earth") print(coord.angular_radius) print(sunpy.coordinates.sun.angular_radius(coord.obstime)) assert_quantity_allclose(coord.angular_radius,sunpy.coordinates.sun.angular_radius(coord.obstime))

when the observer is earth it works fine, but if changed to moon it was giving error

Well, that's because sunpy.coordinates.sun.angular_radius() is expressly for an Earth observer. An observer on the Moon would be a different distance from the Sun, so the angular radius is different from what is returned by sunpy.coordinates.sun.angular_radius().

Sure, I have added the cases along with the changes

changelog/5191.trivial.rst

sunpy/coordinates/tests/test_frames.py

nabobalis · 2021-05-18T07:54:05Z

sunpy/coordinates/tests/test_frames.py

@@ -427,3 +428,20 @@ def test_skycoord_hpc(args, kwargs):
        hgs = sc.transform_to("heliographic_stonyhurst")

    assert isinstance(hgs.frame, HeliographicStonyhurst)
+
+
+def test1_ang_rad():


Can you fix the names of these tests?

nabobalis · 2021-05-18T07:54:21Z

sunpy/coordinates/tests/test_frames.py

@@ -3,6 +3,7 @@

 import numpy as np
 import pytest
+import sunpy


Why was this imported?

nabobalis · 2021-05-18T08:50:56Z

sunpy/coordinates/tests/test_frames.py

+        assert_quantity_allclose(coord.angular_radius, sun.angular_radius(coord.obstime))
+
+
+def test3_ang_radius_no_obstime():


Forgot this one?

sunpy/coordinates/tests/test_frames.py

ayshih

Looks good to me

Anubhav1107 requested a review from a team as a code owner April 6, 2021 13:01

ayshih marked this pull request as draft April 6, 2021 13:19

Anubhav1107 marked this pull request as ready for review April 6, 2021 13:30

nabobalis marked this pull request as draft April 6, 2021 13:32

nabobalis added coordinates Affects the coordinates submodule No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) labels Apr 6, 2021

ayshih requested changes Apr 6, 2021

View reviewed changes

sunpy/coordinates/frames.py Outdated Show resolved Hide resolved

sunpy/coordinates/frames.py Outdated Show resolved Hide resolved

sunpy/coordinates/frames.py Outdated Show resolved Hide resolved

ayshih reviewed May 15, 2021

View reviewed changes

sunpy/coordinates/frames.py Show resolved Hide resolved

sunpy/coordinates/frames.py Show resolved Hide resolved

Anubhav1107 marked this pull request as ready for review May 16, 2021 13:32

Anubhav1107 requested a review from a team as a code owner May 16, 2021 13:37

Anubhav1107 requested review from ayshih and removed request for a team May 16, 2021 13:38

nabobalis reviewed May 16, 2021

View reviewed changes

sunpy/coordinates/tests/test_frames.py Outdated Show resolved Hide resolved

nabobalis reviewed May 16, 2021

View reviewed changes

sunpy/coordinates/tests/test_frames.py Outdated Show resolved Hide resolved

nabobalis removed the request for review from ayshih May 17, 2021 08:15

nabobalis added the Needs Review Needs reviews before merge label May 17, 2021

nabobalis reviewed May 17, 2021

View reviewed changes

changelog/5191.trivial.rst Outdated Show resolved Hide resolved

nabobalis reviewed May 17, 2021

View reviewed changes

sunpy/coordinates/frames.py Outdated Show resolved Hide resolved

nabobalis reviewed May 17, 2021

View reviewed changes

sunpy/coordinates/frames.py Outdated Show resolved Hide resolved

nabobalis reviewed May 17, 2021

View reviewed changes

sunpy/coordinates/frames.py Show resolved Hide resolved

ayshih reviewed May 17, 2021

View reviewed changes

changelog/5191.trivial.rst Outdated Show resolved Hide resolved

nabobalis reviewed May 17, 2021

View reviewed changes

sunpy/coordinates/tests/test_frames.py Outdated Show resolved Hide resolved

nabobalis reviewed May 17, 2021

View reviewed changes

sunpy/coordinates/tests/test_frames.py Show resolved Hide resolved

nabobalis reviewed May 17, 2021

View reviewed changes

sunpy/coordinates/tests/test_frames.py Outdated Show resolved Hide resolved

nabobalis reviewed May 18, 2021

View reviewed changes

sunpy/coordinates/tests/test_frames.py Outdated Show resolved Hide resolved

nabobalis reviewed May 18, 2021

View reviewed changes

sunpy/coordinates/tests/test_frames.py Outdated Show resolved Hide resolved

nabobalis reviewed May 18, 2021

View reviewed changes

sunpy/coordinates/tests/test_frames.py Outdated Show resolved Hide resolved

nabobalis reviewed May 18, 2021

View reviewed changes

sunpy/coordinates/tests/test_frames.py Outdated Show resolved Hide resolved

ayshih changed the title ~~added solar_angular_radius~~ Added a angular_radius convenience property to Helioprojective May 18, 2021

nabobalis force-pushed the master branch 2 times, most recently from 374bf6d to bfebec8 Compare June 2, 2021 17:05

added solar_angular_radius

73b1f94

nabobalis force-pushed the master branch from bfebec8 to 73b1f94 Compare June 2, 2021 17:06

dstansby approved these changes Jun 2, 2021

View reviewed changes

nabobalis requested a review from ayshih June 2, 2021 17:53

nabobalis requested review from ayshih and removed request for ayshih June 18, 2021 15:39

ayshih approved these changes Jun 18, 2021

View reviewed changes

dstansby merged commit 9c728e7 into sunpy:main Jun 18, 2021

dstansby removed the Needs Review Needs reviews before merge label Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a `angular_radius` convenience property to `Helioprojective` #5191

Added a `angular_radius` convenience property to `Helioprojective` #5191

Anubhav1107 commented Apr 6, 2021

pep8speaks commented Apr 6, 2021 •

edited

ayshih left a comment

Anubhav1107 commented Apr 6, 2021

ayshih left a comment

Anubhav1107 commented Apr 6, 2021

dstansby commented Apr 24, 2021

Anubhav1107 commented May 12, 2021

Anubhav1107 commented May 13, 2021

nabobalis commented May 13, 2021 •

edited

ayshih commented May 15, 2021

Anubhav1107 commented May 16, 2021

nabobalis commented May 16, 2021

Anubhav1107 commented May 16, 2021

nabobalis commented May 16, 2021

nabobalis May 16, 2021

Anubhav1107 commented May 16, 2021

nabobalis commented May 16, 2021

ayshih May 17, 2021

Anubhav1107 May 17, 2021

Anubhav1107 May 17, 2021

ayshih May 17, 2021

Anubhav1107 May 17, 2021

ayshih May 17, 2021

Anubhav1107 May 17, 2021

nabobalis May 18, 2021

nabobalis May 18, 2021

nabobalis May 18, 2021

ayshih left a comment

		assert_quantity_allclose(coord.angular_radius, sun.angular_radius(coord.obstime))


		def test3_ang_radius_no_obstime():

Added a angular_radius convenience property to Helioprojective #5191

Added a angular_radius convenience property to Helioprojective #5191

Conversation

Anubhav1107 commented Apr 6, 2021

Description

pep8speaks commented Apr 6, 2021 • edited

Comment last updated at 2021-06-02 17:06:03 UTC

ayshih left a comment

Choose a reason for hiding this comment

Anubhav1107 commented Apr 6, 2021

ayshih left a comment

Choose a reason for hiding this comment

Anubhav1107 commented Apr 6, 2021

dstansby commented Apr 24, 2021

Anubhav1107 commented May 12, 2021

Anubhav1107 commented May 13, 2021

nabobalis commented May 13, 2021 • edited

ayshih commented May 15, 2021

Anubhav1107 commented May 16, 2021

nabobalis commented May 16, 2021

Anubhav1107 commented May 16, 2021

nabobalis commented May 16, 2021

Choose a reason for hiding this comment

Anubhav1107 commented May 16, 2021

nabobalis commented May 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayshih left a comment

Choose a reason for hiding this comment

Added a `angular_radius` convenience property to `Helioprojective` #5191

Added a `angular_radius` convenience property to `Helioprojective` #5191

pep8speaks commented Apr 6, 2021 •

edited

nabobalis commented May 13, 2021 •

edited