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 into
astropy: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 into
astropy:masterfrom
Cadair:whysobroken

Conversation

@Cadair

@Cadair Cadair commented Jun 15, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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

Cadair commented Jun 15, 2017

Copy link
Copy Markdown
Member Author

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

@Cadair

Cadair commented Jun 15, 2017

Copy link
Copy Markdown
Member Author

@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

pllim commented Jun 15, 2017

Copy link
Copy Markdown
Member

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

Cadair commented Jun 15, 2017

Copy link
Copy Markdown
Member Author

@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

adrn commented Jun 15, 2017

Copy link
Copy Markdown
Member

@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

Cadair commented Jun 15, 2017

Copy link
Copy Markdown
Member Author

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

@mhvk

mhvk commented Jun 17, 2017

Copy link
Copy Markdown
Contributor

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...

@astrofrog astrofrog mentioned this pull request Jun 17, 2017
Merged
@eteq eteq closed this in #6236 Jun 19, 2017
@eteq

eteq commented Jun 19, 2017

Copy link
Copy Markdown
Member

@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

Reviewers

@eteq eteq Awaiting requested review from eteq

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@Cadair @pllim @adrn @mhvk @eteq @bsipocz