Skip to content
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 the representation of observers in frame repr #2723

Merged
merged 12 commits into from Aug 22, 2018

Conversation

Projects
None yet
6 participants
@Cadair
Copy link
Member

commented Aug 15, 2018

This fixes the issue raised by @tiagopereira in #2706 that the observer representation is very verbose when a more useful representation is "earth". This fixes that.

Cadair added some commits Aug 15, 2018

Override __str__ on HeliographicStonyhurst to support observers.
This enables us to print "earth" if the observer is in fact earth and not the
whole repr of the frame

@Cadair Cadair added this to the 0.9.3 milestone Aug 15, 2018

@pep8speaks

This comment has been minimized.

Copy link

commented Aug 15, 2018

Hello @Cadair! Thanks for updating the PR.

Line 104:101: E501 line too long (105 > 100 characters)
Line 107:101: E501 line too long (106 > 100 characters)
Line 209:101: E501 line too long (105 > 100 characters)
Line 215:101: E501 line too long (105 > 100 characters)
Line 223:5: E303 too many blank lines (2)
Line 289:101: E501 line too long (132 > 100 characters)
Line 292:101: E501 line too long (109 > 100 characters)
Line 295:101: E501 line too long (141 > 100 characters)
Line 346:101: E501 line too long (180 > 100 characters)
Line 350:101: E501 line too long (156 > 100 characters)

Line 53:101: E501 line too long (112 > 100 characters)
Line 118:101: E501 line too long (110 > 100 characters)
Line 127:101: E501 line too long (112 > 100 characters)
Line 141:101: E501 line too long (114 > 100 characters)
Line 143:101: E501 line too long (180 > 100 characters)
Line 276:9: E129 visually indented line with same indent as next logical line
Line 284:101: E501 line too long (104 > 100 characters)
Line 318:101: E501 line too long (102 > 100 characters)

Comment last updated on August 22, 2018 at 17:34 Hours UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Aug 15, 2018

Thanks for the pull request @Cadair! Everything looks great!

@Cadair Cadair added [BugFix] and removed Bug(?) labels Aug 15, 2018

@sunpy sunpy deleted a comment from sunpy-bot bot Aug 15, 2018

@sunpy sunpy deleted a comment from sunpy-bot bot Aug 15, 2018

@sunpy sunpy deleted a comment from sunpy-bot bot Aug 15, 2018

@sunpy sunpy deleted a comment from sunpy-bot bot Aug 15, 2018

Cadair added some commits Aug 15, 2018

@Cadair

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

This is what this does btw:

from astropy.coordinates import SkyCoord
from sunpy.coordinates import frames
import astropy.units as u


sc = SkyCoord(0*u.deg, 0*u.deg, frame=frames.Helioprojective, obstime="2011-01-01", observer="earth")
>>> sc
<SkyCoord (Helioprojective: obstime=2011-01-01 00:00:00, rsun=695508.0 km, observer=earth): (Tx, Ty) in arcsec
    (0., 0.)>
>>> sc.observer
<HeliographicStonyhurst Coordinate (obstime=2011-01-01 00:00:00): (lon, lat, radius) in (deg, deg, AU)
    (0., -2.97718014, 0.98335587)>
>>> print(sc.observer)
earth
>>> sc.observer._observer_body
'earth'

@Cadair Cadair requested review from wafels and tiagopereira Aug 15, 2018

@tiagopereira
Copy link
Member

left a comment

Looks much better to me!

@ayshih ayshih removed the [BugFix] label Aug 17, 2018

@ayshih

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2018

I removed the bugfix label because I don't feel that it is appropriate for this PR.

While I'm a fan of the intent here, I'm concerned about this behavior:

>>> print(sc.observer)
earth

I find it confusing for such a complex object to show such limited output with a print() call. Am I just weird here? I don't know if there's an easy way around this behavior.

I think you should also overload __repr__() to report the observer string.

@Cadair

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2018

I removed the bugfix label because I don't feel that it is appropriate for this PR.

I was also debating this.

Am I just weird here?

yeah I don't like it either.

I don't know if there's an easy way around this behavior.

Not to achieve the objective. The repr for BaseCoordinateFrame and SkyCoord call str() on all the frame attributes. So if we want them to display like that in that context this is the upshot.

I think you should also overload __repr__() to report the observer string.

I deliberately didn't do this to mitigate the issues with print. The repr should just be the normal coordinate object to me, as that is, after all, what the class is. The changing of __str__ is really just a hack.

@Cadair Cadair modified the milestones: 0.9.3, 1.0 Aug 17, 2018

@Cadair Cadair removed the 0.9.x label Aug 17, 2018

@Cadair

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2018

I don't know if there's an easy way around this behavior.

So the only way around this would be to go into Astropy and poke at this method to call out to a _attribute_repr method if it is present rather than __str__.

@ayshih

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2018

There's no strong reason why the repr needs to say only observer=earth except for the parallelism with the input. What if instead the str() call returned <HeliographicStonyhurst Coordinate of Earth>? It's longer, of course, but I think it adds even more clarity while avoiding the confusion of just returning earth.

@ayshih

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2018

In fact, we should just generalize this beyond just observers. It'd be a handy way to name coordinates, regardless of whether they are used as observers. If the coordinate has a name attribute, it'd just stick it into the str()-ed version, e.g.:

<HeliographicStonyhurst Coordinate of Crab Nebula>
<Heliocentric Coordinate of RHESSI>

What do you think?

@Cadair

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2018

I like that its a good idea. Such a good idea in fact I wonder if we should propose upstreaming that...

Cadair added some commits Aug 18, 2018

@Cadair

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2018

@ayshih Take a look now :)

@Cadair

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2018

preview:

image

Cadair added some commits Aug 18, 2018

@ayshih

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

Sweet, I like it!

@wafels

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

The docs will need updating to reflect this, after acceptance.

@ayshih

ayshih approved these changes Aug 22, 2018

@ayshih
Copy link
Contributor

left a comment

Update the changelog text to reflect the more general nature of this change

@wafels

wafels approved these changes Aug 22, 2018

Copy link
Member

left a comment

I'll raise another issue about raising awareness of this feature, perhaps in a gallery example.

@Cadair

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2018

@ayshih changelog updated.

@ayshih

ayshih approved these changes Aug 22, 2018

@Cadair Cadair merged commit e60fb5c into sunpy:master Aug 22, 2018

9 checks passed

ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
codecov/patch 93.33% of diff hit (target 85.39%)
Details
codecov/project Absolute coverage decreased by -0.41% but relative coverage increased by +7.93% compared to 46a9c6b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed

@Cadair Cadair deleted the Cadair:observer_name branch Aug 22, 2018

@tiagopereira

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Am I missing something or this no longer works? With sunpy 0.9.6 and astropy 3.1.2:

from astropy.coordinates import SkyCoord
from sunpy.coordinates import frames
import astropy.units as u

sc = SkyCoord(0*u.deg, 0*u.deg, frame=frames.Helioprojective, obstime="2011-01-01", observer="earth")
>>> sc
<SkyCoord (Helioprojective: obstime=2011-01-01 00:00:00, rsun=695508.0 km, observer=<HeliographicStonyhurst Coordinate (obstime=2011-01-01 00:00:00): (lon, lat, radius) in (deg, deg, AU)
    (0., -2.97718014, 0.98335587)>): (Tx, Ty) in arcsec
    (0., 0.)>
>>> print(sc.observer)
<HeliographicStonyhurst Coordinate (obstime=2011-01-01 00:00:00): (lon, lat, radius) in (deg, deg, AU)
    (0., -2.97718014, 0.98335587)>
>>> sc.observer.object_name
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-6-074b3cd3b55e> in <module>
----> 1 sc.observer._observer_body

~/miniconda/lib/python3.6/site-packages/astropy/coordinates/baseframe.py in __getattr__(self, attr)
   1517         # Prevent infinite recursion here.
   1518         if attr.startswith('_'):
-> 1519             return self.__getattribute__(attr)  # Raise AttributeError.
   1520
   1521         repr_names = self.representation_component_names

AttributeError: 'HeliographicStonyhurst' object has no attribute 'object_name'
@Cadair

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

@tiagopereira This wasn't classed as a bugfix so hasn't been released in the 0.9.z series of releases. It will be included in the next release (1.0).

(I decided to consider __repr__ changes as API changes because it's very easy for them to cause peoples doctests to fail, numpy did this on a previous release and it caused us chaos).

@tiagopereira

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

@Cadair thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.