Frames from Shipp et al 2019

[nstarman]

Is this is of interest?

Couple possible simplifications:

• [x] common base class
• [x] use format_doc to simplify the docstrings

Checklist

• [ ] Did you add tests?
• [ ] Did you add documentation for your changes?
• [ ] Did you reference any relevant issues?
• [x] Did you add a changelog entry? (see CHANGES.rst)
• [ ] Are the CI tests passing?
• [x] Is the milestone set?

[adrn]

Sure! I think there is some duplication with Cecilia's work in galstreams, but one doesn't always need the full galstreams footprint, so I'm happy to include these here. ~~I left one question as a code comment for consideration.~~ I was about to say "can you make a common base class to simplify all the boilerplate code?" but just noticed you said that in your opening comments :). But yes, please do that! I think it makes sense to have something like a "BaseStreamFrame" or something.

[adrn]

Oh, and could you add a changelog entry? Thanks for this!

[nstarman]

I made a common base class, but only applied it to the new frames. I didn't want to refactor other frames, which are different in small details (e.g. the representation wrapping mechanics).

Also, what tests do you think should be added? I was thinking at minimum adding 2 point tests that a) the origin and b) a point on the stream map to the correct coordinates in ICRS vs the stream frame.

Possible followups are:

• moving the other frames to _builtin_frames (or the contents of _builtin_frames out, depending on how you want to PEP8 this)
• use BaseStreamFrame for other frames.

[adrn]

I totally forgot about this! I'm going to release v1.8 this week -- should we get this in? Rebase and remove the draft status?

[nstarman]

Hm. I've discovered some inconsistencies. The rotation matrices in this PR are from https://iopscience.iop.org/article/10.3847/1538-4357/ab44bf/pdf. To make the tests I went to https://iopscience.iop.org/article/10.3847/1538-4357/aacdab/pdf, the paper that includes a pole and two end-points on the stream. The endpoints should end up with phi2 of 0.

Highlighting the Atlas frame:

>>> c = ICRS(ra=-6.3 * u.deg, dec=-59.7 * u.deg)
>>> c.transform_to(AtlasShipp19())
<AtlasShipp19 Coordinate: (phi1, phi2) in deg
    (-8.64534171, -35.83870908)>

I thought one possibility is that the rotation matrix and all coordinates are actually FK5 (not ICRS) because the poles and endpoints in https://iopscience.iop.org/article/10.3847/1538-4357/aacdab/pdf are all listed as alpha2000, dec2000. While this might be true (and if it is then this PR needs updated), it doesn't fix the issue. Nor does transposing the rotation matrix to address an active versus passive transformation difference between the paper and Astropy's machinery.

When I define a GreatCircleICRS from the endpoints and test the pole it is much better (but still a little off suggesting rounding differences). So there is internal consistency in https://iopscience.iop.org/article/10.3847/1538-4357/aacdab/pdf, but I'm not getting how that translated into https://iopscience.iop.org/article/10.3847/1538-4357/ab44bf/pdf. Let's ask!

There's something strange happening with the rotation matrix.

[nstarman]

As a related note, directly specifying a rotation matrix is inferior to defining the matrix from end-points, the pole, Euler angles, etc, because the rotation matrix must be SO(3) and due to numerical precision none actually are. This normally isn't an issue, but specifying a rotation matrix with float64 precision means that it isn't a rotation matrix for arbitrary-precision math! At some point we should address this in Astropy and downstream, where it should be possible to increase (or decrease) the precision of everything. In the meantime, I can change the classes to be ...Shipp18 and use the endpoints. Is that worth it?