Skip to content

Implement Finite Difference Velocity Transformations#13

Closed
eteq wants to merge 189 commits intoadrn:coordinates/frame-velocity-supportfrom
eteq:coordinates/finite-diff-vels
Closed

Implement Finite Difference Velocity Transformations#13
eteq wants to merge 189 commits intoadrn:coordinates/frame-velocity-supportfrom
eteq:coordinates/finite-diff-vels

Conversation

@eteq
Copy link

@eteq eteq commented Jun 15, 2017

WIP...

@eteq eteq changed the title Coordinates/finite diff vels Implement Finite Difference Velocity Transformations Jun 15, 2017
@eteq eteq force-pushed the coordinates/finite-diff-vels branch from dc15777 to 8f82c3d Compare June 15, 2017 16:03
@adrn
Copy link
Owner

adrn commented Jun 15, 2017

Sick!! Have you tried this going from GCRS to ICRS to see if it produces the correct Barycentric velocity correction?

@eteq
Copy link
Author

eteq commented Jun 15, 2017

Have you tried this going from GCRS to ICRS to see if it produces the correct Barycentric velocity correction?

Just what I'm looking at right now actually! It doesn't work, but I think due to some straightforward-to-fix bugs. Will push a few more tests up soon hopefully, including that.

@adrn adrn force-pushed the coordinates/frame-velocity-support branch 2 times, most recently from 856e89b to 68457ec Compare June 16, 2017 04:27
@eteq eteq force-pushed the coordinates/finite-diff-vels branch 2 times, most recently from 32c8c86 to 52f28b4 Compare June 16, 2017 07:50
@eteq eteq mentioned this pull request Jun 16, 2017
Merged
eteq added 15 commits June 16, 2017 18:27
@eteq
add helio velocities correction
7ffd296
@eteq
add tests
7e5b1ef
@eteq
fixes for code based on tests
34d70fe
@eteq
work around apparent matrix_product bug
f37e52e
@eteq
update to radial_velocity_correction and give bary option
7c5f914
@eteq
update to new implementation based on @mhvk and @StuartLittlefair rev…
c45114a 
…iews
@eteq
rename velocities to something more specific
674fa31
@eteq
slight reordering in docstring to make syntax highlighters less confused
d80e5d5 
anything that sees strings as regex-like gets confused by [\]
@eteq
fixes to velcorr algorithm
104c880
@eteq
make velcorr publicly visible as SkyCoord method
e01247d
@eteq
finish ephemeris argument
ddd2079
@eteq
clean up process in Skycoord.radial_velocity_correction
b1d9c08
@eteq
fix left-behind ephemeris
68bd434
@eteq
add barycorr test
c9b4cfe
@eteq
add multi-obstime tests and related fix
a006748
@eteq eteq force-pushed the coordinates/finite-diff-vels branch 2 times, most recently from 17fbb3e to 867b685 Compare June 18, 2017 05:31
@adrn adrn force-pushed the coordinates/frame-velocity-support branch from bc3247b to 472f385 Compare June 18, 2017 19:26
@eteq
Merge pull request astropy#6219 from adrn/coordinates/frame-velocity-…
2279f1c 
…support

Add velocity support to coordinate frames and transformations
adrn
adrn reviewed Jun 27, 2017
View reviewed changes
astropy/coordinates/tests/test_finite_difference_velocities.py Outdated
class LSR2(LSR):
obstime = TimeFrameAttribute(default=J2000)

dt = 1*u.s
Copy link
Owner

@adrn adrn Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overrides the dt passed in to the test function.

Copy link
Author

@eteq eteq Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops! fortunately it passes either way...

adrn
adrn reviewed Jun 27, 2017
View reviewed changes
astropy/coordinates/tests/test_finite_difference_velocities.py
ax2.set_title('total')


sd = frame.data.differentials['s'].represent_as(SphericalDifferential, frame.data)
Copy link
Owner

@adrn adrn Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SphericalDifferential isn't imported

adrn
adrn reviewed Jun 27, 2017
View reviewed changes
astropy/coordinates/tests/test_finite_difference_velocities.py Outdated
# if its a lot bigger than this, finite-difference noise has ruined things
assert np.ptp(rv) < 65*u.km/u.s

def diff_info_plot(frame, times):
Copy link
Owner

@adrn adrn Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

times should be time to work with variables in function.

@adrn
Copy link
Owner

adrn commented Jun 27, 2017

You should be able to implement a test that checks this against some of the matrix transforms. For example:

def test_faux_fk5_galactic():
    class Galactic2(Galactic):
        pass

    dt = 1*u.s

    @frame_transform_graph.transform(FunctionTransformWithFiniteDifference,
                                     FK5, Galactic2, finite_difference_dt=dt,
                                     symmetric_finite_difference=True,
                                     finite_difference_frameattr_name=None)
    def fk5_to_gal2(fk5_coo, gal_frame):
        trans = DynamicMatrixTransform(fk5_to_gal, FK5, Galactic2)
        return trans(fk5_coo, gal_frame)

    @frame_transform_graph.transform(FunctionTransformWithFiniteDifference,
                                     Galactic2, ICRS, finite_difference_dt=dt,
                                     symmetric_finite_difference=True,
                                     finite_difference_frameattr_name=None)
    def gal2_to_fk5(gal_coo, fk5_frame):
        trans = DynamicMatrixTransform(_gal_to_fk5, Galactic2, FK5)
        return trans(gal_coo, fk5_frame)

    c1 = FK5(ra=150*u.deg, dec=-17*u.deg, radial_velocity=83*u.km/u.s,
             pm_ra_cosdec=-41*u.mas/u.yr, pm_dec=16*u.mas/u.yr,
             distance=150*u.pc)
    c2 = c1.transform_to(Galactic2)
    c3 = c1.transform_to(Galactic)
    
    # compare the values below
    c2.pm_l_cosb.to(u.mas/u.yr), c2.pm_b.to(u.mas/u.yr)
    c3.pm_l_cosb.to(u.mas/u.yr), c3.pm_b.to(u.mas/u.yr)

I was finding ~0.3% accuracy.

@eteq
Copy link
Author

eteq commented Jun 27, 2017

Alright, I think all the comments above have been addressed!

eteq added 21 commits June 27, 2017 01:33
@eteq
add FunctionTransformWithFiniteDifference
e567cc5
@eteq
implemented test, but not working due to upstream
356a407
@eteq
Fixes to make finite differences work
a9215ad
@eteq
functioning test (but wrong answer)
bfb5a45
@eteq
finite differences working!
d1f5c2b
@eteq
get intrinsic differntial shift working
8af183f
@eteq
broaden faux lsr tests and associated fixes
ee00a91
@eteq
extend finite difference to all function transforms except skyoffset
0d21ae6
@eteq
add vhelio check in finite differences
ed5686b
@eteq
update finite differences to work for GCRS and partly AltAz
136a341
@eteq
wide range of fixes to finite differences revealed by gcrs tests
2c6f195
@eteq
implement more complete set of tests
44e50d3
@eteq
Add warning for FunctionTransform that fails to address differentials
d5cc1ce
@eteq
make FunctionTransformer call cleaner
77533f0
@eteq
Update code to be consistent with newest velocity frame branch
089e7d5
@eteq
Minor test adjustments
d5bbc2c
@eteq
Optimize finite difference a bit more
6a7adcd
@eteq
test and typo clarifications
2f001d6
@eteq
Updates to finite difference logic from @adrn's review
34cd620
@eteq
fixes in tests from @adrn review
f438550
@eteq
test for finite difference vs affine
6bfd3d0
@eteq eteq force-pushed the coordinates/finite-diff-vels branch from e2d4f1c to 6bfd3d0 Compare June 27, 2017 05:33
@eteq
Copy link
Author

eteq commented Jun 27, 2017

Closing this in favor of astropy#6226

@eteq eteq closed this Jun 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adrn adrn adrn left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.