Fix construction of HG frames to be 3D

[Cadair]

Fixes #3350

[ghost]

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

[ayshih]

While this PR does fix the issue, I suspect that there might be a better/cleaner way to fix it. Or I could just be imagining things. I'd like to do some tinkering before letting this through, unless a bugfix release is just around the corner.

[Cadair]

@ayshih It's not the end of the world if this floats for a little while, but it is breaking me messing around with #3353.

I thought a decent amount about this, and I am pretty sure this is the best way. When you construct the frame with an explict SphericalRepresentation it seems to me the only logical thing to do is to throw it through our code to add R.

[ayshih]

I just PR-ed to your branch. After poking around a bunch, I've decided that the whole approach to keyword checking and represent_as was unnecessarily complicated. (The represent_as stuff is my fault.) Instead it will now always upgrade to 3D, as already promised in the docstring. If a user chooses to specify a representation_type of UnitSphericalRepresentation, that's what they'll be shown, with component attributes to match, but the underlying data will be 3D.

[pep8speaks]

Hello @Cadair! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file sunpy/coordinates/frames.py:

Line 225:13: E129 visually indented line with same indent as next logical line

Comment last updated at 2019-10-19 12:40:26 UTC

[Cadair]

Do the changes to the represent_as function have any effect here? Is this still just a bug fix?

[ayshih]

In terms of user-facing API, the change here is what .data returns. Previously, if a user specifically defined the representation_type, the class would try to preserve the underlying data and its representation, even if it was 2D rather than 3D. The .represent_as() stuff was there so that the class was still able to work with the underlying data as if it were 3D. Thus, previously, .data could still return a 2D representation, although this behavior was entirely undocumented. (The bug identified in the corresponding issue was because the SkyCoord machinery can specify a representation_type that is different from the type of the representation.) I think this still qualifies as a bug fix because these shenanigans were in violation of the docstring, which says that the data will always be converted to 3D.

[Cadair]

Could you perhaps suggest some appropriate wording for the changelog just to make that obvious to everyone? (or I can and then you can comment on it?)

[ayshih]

Here's a wordier version:

Fixed situations where 2D coordinates provided to `~sunpy.coordinates.frames.HeliographicStonyhurst` and `~sunpy.coordinates.frames.HeliographicCarrington` were not converted to 3D as intended.  Furthermore, the stored data will always be the post-conversion, 3D version.

[backporting]

The backport to 1.0 failed:

Commits ["956d96258d1b011ef036378304b65fb643c31571","87e96ae32e273b89e1eec18364d6f025f8ea7d8c","b13422b36a587ff992bf07cd630dbb61ed1b0da4","d9eb5e846b3c3eaa73f142e3ea000918745d5560","85114e9928a2f1347c18107ee929232189bda559","96b1301a575aa5ea22df2486da8322341f4ea56a"] could not be cherry-picked on top of 1.0

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub.
git fetch
# Create new working tree.
git worktree add .worktrees/backport 1.0
# Navigate to the new directory.
cd .worktrees/backport
# Cherry-pick all the commits of this pull request and resolve the likely conflicts.
git cherry-pick 956d96258d1b011ef036378304b65fb643c31571 87e96ae32e273b89e1eec18364d6f025f8ea7d8c b13422b36a587ff992bf07cd630dbb61ed1b0da4 d9eb5e846b3c3eaa73f142e3ea000918745d5560 85114e9928a2f1347c18107ee929232189bda559 96b1301a575aa5ea22df2486da8322341f4ea56a
# Create a new branch with these backported commits.
git checkout -b backport-3351-to-1.0
# Push it to GitHub.
git push --set-upstream origin backport-3351-to-1.0
# Go back to the original working tree.
cd ../..
# Delete the working tree.
git worktree remove .worktrees/backport

Then, create a pull request where the base branch is 1.0 and the compare/head branch is backport-3351-to-1.0.

[Cadair]

Point of order, it seems that this backport failed because of the merge commit.