-
Notifications
You must be signed in to change notification settings - Fork 53
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
Frames from Shipp et al 2019 #325
base: main
Are you sure you want to change the base?
Conversation
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. |
Oh, and could you add a changelog entry? Thanks for this! |
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com> Signed-off-by: nstarman <nstarman@users.noreply.github.com>
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:
|
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? |
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. 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 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. |
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 |
Is this is of interest?
Couple possible simplifications:
format_doc
to simplify the docstringsChecklist
CHANGES.rst
)