Skip to content

Support Velocities in coordinates by having transforms that use finite differences#6226

Merged
eteq merged 26 commits intoastropy:masterfrom
eteq:coordinates/finite-diff-vels
Jun 27, 2017
Merged

Support Velocities in coordinates by having transforms that use finite differences#6226
eteq merged 26 commits intoastropy:masterfrom
eteq:coordinates/finite-diff-vels

Conversation

@eteq
Copy link
Member

@eteq eteq commented Jun 16, 2017

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_limits test). 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 allows FunctionTransforms 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 own FunctionTransform that they should update it if it will ever be used with differentials.

@eteq eteq added this to the v2.0.0 milestone Jun 16, 2017
@mhvk
Copy link
Contributor

mhvk commented Jun 16, 2017

@eteq - sorry I didn't get to this yet and am now too tired. Hopefully tomorrow...

@eteq
Copy link
Member Author

eteq commented Jun 18, 2017

Oh and note that the failures here are all (or almost all?) due to the branches its based on

@eteq eteq force-pushed the coordinates/finite-diff-vels branch 2 times, most recently from 17fbb3e to 867b685 Compare June 18, 2017 05:31
@eteq
Copy link
Member Author

eteq commented Jun 18, 2017

@mhvk - if you have time you might want to have a high-level look at this. Conceptually/API-wise I think its probably more straightforward then @adrn's PRs. (As it stands right now it needs to rebase for #6169 and #6219, but awaiting those to be ready before adjusting this)

@eteq eteq changed the title Support Velocities in coordinates with finite differences Support Velocities in coordinates by having transforms that use finite differences Jun 19, 2017
@eteq eteq force-pushed the coordinates/finite-diff-vels branch from 867b685 to 27b8552 Compare June 23, 2017 05:37
@eteq
Copy link
Member Author

eteq commented Jun 23, 2017

(Note that this has been rebased to #6219, so it should be pretty straightforward once we get #6219 in...)

@eteq eteq force-pushed the coordinates/finite-diff-vels branch from 27b8552 to 5d3f0cb Compare June 26, 2017 22:55
@eteq
Copy link
Member Author

eteq commented Jun 26, 2017

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)

@eteq
Copy link
Member Author

eteq commented Jun 27, 2017

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.

@eteq eteq added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Jun 27, 2017
@eteq
Copy link
Member Author

eteq commented Jun 27, 2017

Alright, tests are all passing, so given the review from adrn#13, I'll merge this now (contingent on the same conditions as #6219, though)

@eteq eteq merged commit 90d72c0 into astropy:master Jun 27, 2017
@eteq eteq deleted the coordinates/finite-diff-vels branch June 27, 2017 07:22
eteq added a commit that referenced this pull request Jun 27, 2017
Support Velocities in coordinates by having transforms that use finite differences
@MSeifert04 MSeifert04 removed the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants