Support Velocities in coordinates by having transforms that use finite differences#6226
Merged
eteq merged 26 commits intoastropy:masterfrom Jun 27, 2017
Merged
Conversation
1 task
Contributor
|
@eteq - sorry I didn't get to this yet and am now too tired. Hopefully tomorrow... |
Member
Author
|
Oh and note that the failures here are all (or almost all?) due to the branches its based on |
17fbb3e to
867b685
Compare
Member
Author
This was referenced Jun 20, 2017
867b685 to
27b8552
Compare
Member
Author
27b8552 to
5d3f0cb
Compare
Member
Author
|
This has been rebased yet again on #6219 and should be all passing. Hence it could be ready to go if #6219 also is. (and for a test "preview" see https://travis-ci.org/eteq/astropy/builds/247302995) |
e2d4f1c to
6bfd3d0
Compare
Member
Author
|
Somewhat confusingly, the review for this has been in adrn#13 because the diff was easier to read there. But this has been reviewed there and I believe is now set to go assuming the tests pass following the rebase on master. |
Member
Author
eteq
added a commit
that referenced
this pull request
Jun 27, 2017
Support Velocities in coordinates by having transforms that use finite differences
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is the fourth (and last ) in a series by @adrn, @mhvk, and myself trying to support velocities in astropy.coordinates. This PR is based on #6219, so will need to be merged after that one, and has a lot of commits from the series of PRs. To see a cleaner diff with just these changes, take a look at adrn#13.
The main purpose of this particular segment of the work is implementing a new transformation that uses finite difference techniques to compute velocities in coordinate frames. It also replaces most of the Astropy transformation machinery to use that new transformation, supporting transforming between the various astropy frames and getting correct velocities out.
It's all working (and there are explicit tests for the main frames of practical use), but there is a flaw: for objects more distant then ~10 kpc, solar system velocity effects (most critically, the heliocentric correction) start getting swamped in machine precision noise because the km-kpc dynamic range is just too much. (This is demonstrated in the
test_numerical_limitstest). For closer objects this isn't really a problem.This can be fixed in principal by clever finite difference techniques or even just capping the distance as seen in the finite difference transform. However, the details of this probably cannot be worked out in time for the feature freeze. So I propose (if all is well) merging this (after the remainder of the velocity stack of PRs is in), but also merge an updated version of #5752 (which I can easily get to tomorrow). The docs will reflect this plan in that there will be a big red "velocity support is experimental" warning in the coordinates docs, and the functions from #5752 will also mention that their implementation might change to use the coordinates machinery ones the finite difference issues have been ironed out.
One other change this makes is that there is now a warning for
FunctionTransform's that drop differentials. This still allowsFunctionTransforms to set differentials themselves, but is there to make it clear that "legacy"FunctionTransforms should be updated to do something with the differentials. This is debatable behavior... it would be easy to just have it silently drop the differentials on the assumption that this is the intended behavior. But I think its better this way to remind anyone whos implemented their ownFunctionTransformthat they should update it if it will ever be used with differentials.