Skip to content

#6182 Modified the behaviour of realize_frame so it changes the representation attribute of the frame#6208

Closed
Cadair wants to merge 1 commit intoastropy:masterfrom
Cadair:whysobroken
Closed

#6182 Modified the behaviour of realize_frame so it changes the representation attribute of the frame#6208
Cadair wants to merge 1 commit intoastropy:masterfrom
Cadair:whysobroken

Conversation

@Cadair
Copy link
Member

@Cadair Cadair commented Jun 15, 2017

SunPy sets the representation of the frame inside the __init__ for the frame to work around Longitude wrapping. #6182 changed the behaviour of realize_frame so that it now explicitly set's the representation of the new frame to that of the old frame, which means we can't change the representation of the frame in the __init__ in the correct way.

Offending line in SunPy: https://github.com/sunpy/sunpy/blob/master/sunpy/coordinates/frames.py#L338

Offending commit in Astropy: 1729467

This fix is a bit facetious in that I am sure it will break something, I am open to suggestions for an actual fix to this problem!

@Cadair
Copy link
Member Author

Cadair commented Jun 15, 2017

An alternative fix to this might be to revert the changes to realize_frame so it does not use _apply.

@Cadair
Copy link
Member Author

Cadair commented Jun 15, 2017

@eteq You broke it ;)

@astrofrog astrofrog requested a review from eteq June 15, 2017 09:59
@bsipocz bsipocz added coordinates Affects-dev PRs and issues that do not impact an existing Astropy release Bug labels Jun 15, 2017
@pllim pllim added this to the v2.0.0 milestone Jun 15, 2017
@pllim
Copy link
Member

pllim commented Jun 15, 2017

I am milestoning this to v2 since it won't be "affect dev" anymore after release. @Cadair , do you plan to address the test failures?

@Cadair
Copy link
Member Author

Cadair commented Jun 15, 2017

@pllim There needs to be another solution really, but I don't understand enough of the reasons behind the change in the first place.

@adrn
Copy link
Member

adrn commented Jun 15, 2017

@Cadair Just so I understand: the SunPy code is needed because you'd like to be able to enforce that the representation becomes a SphericalWrap180Representation if a SphericalRepresentation is passed in?

@Cadair
Copy link
Member Author

Cadair commented Jun 15, 2017

Yes, and for another frame we use a similar bit of logic to fill the radius coordinate with a default value.

@mhvk
Copy link
Contributor

mhvk commented Jun 17, 2017

As this is in part a problem from me urging @eteq to use _apply -- I think this is not too difficult to solve; the problem is indeed that _apply sets the representation after running through the initializer. This really should not be necessary. I'll see if I can make a more elegant fix. In the meantime, @Cadair, if you'd have a minimal test case that doesn't rely on sunpy...

@eteq
Copy link
Member

eteq commented Jun 19, 2017

@Cadair - I think you confirmed in #6236 that this fixes it, but if not feel free to re-open another issue on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release Bug coordinates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants