Skip to content

Direct icrs altaz - #11079

Merged
StuartLittlefair merged 5 commits into
astropy:masterfrom
StuartLittlefair:direct_icrs_altaz
Nov 25, 2020
Merged

Direct icrs altaz#11079
StuartLittlefair merged 5 commits into
astropy:masterfrom
StuartLittlefair:direct_icrs_altaz

Conversation

@StuartLittlefair

@StuartLittlefair StuartLittlefair commented Nov 24, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR fixes #10887 by implementing a 'direct' transformation between ICRS and AltAz. In fact, this transform routes via CIRS, but by doing it in one routine we avoid the overhead of computing the astrometry context twice, and the overhead in creating a SkyCoord object for each step.

This results in a modest speedup. Running @maxnoe's performance profiling from #10647 (comment), we find:

No Interpolation took 8.8879 s
Interpolation with  10.0 s   resolution took 0.9227 s
Interpolation with   5.0 min resolution took 0.2207 s
Interpolation with  30.0 min resolution took 0.1623 s

compared to the current master:

No Interpolation took 9.7546 s
Interpolation with  10.0 s   resolution took 1.0634 s
Interpolation with   5.0 min resolution took 0.2984 s
Interpolation with  30.0 min resolution took 0.2053 s

The only remaining puzzle is that I am unable to reproduce the high precision for GCRS->AltAz introduced in #11073. What is a mystery is that the transforms in that test are unaltered - they still go through the untouched GCRS->CIRS->AltAz chain. So I am submitting this PR to see if tests pass remotely and the local failure is something to do with my setup.

@StuartLittlefair StuartLittlefair added this to the v4.3 milestone Nov 24, 2020
mhvk
mhvk reviewed Nov 24, 2020
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good modulo some comments to avoid copies (which are not going to matter much, but one might as well). It seems surprising that the speed-up is relatively minimal. Are we missing something? Though looking at some quick timings, I guess it is just that CIRS->AltAz is just much more expensive.

p.s. The failures all seem directly related to the PR, but the fact that you get local failures is a bit worrisome.

Comment thread astropy/coordinates/builtin_frames/icrs_observed_transforms.py Outdated
Comment thread astropy/coordinates/builtin_frames/icrs_observed_transforms.py Outdated
Comment thread astropy/coordinates/builtin_frames/icrs_observed_transforms.py Outdated
Comment thread astropy/coordinates/builtin_frames/icrs_observed_transforms.py Outdated
Comment thread astropy/coordinates/builtin_frames/icrs_observed_transforms.py Outdated
Comment thread astropy/coordinates/builtin_frames/icrs_observed_transforms.py Outdated
Comment thread astropy/coordinates/builtin_frames/icrs_observed_transforms.py Outdated
Comment thread astropy/coordinates/builtin_frames/icrs_observed_transforms.py
@StuartLittlefair

StuartLittlefair commented Nov 24, 2020

Copy link
Copy Markdown
Contributor Author

Quick work @mhvk! This is a little faster now as a result:

No Interpolation took 8.8463 s
Interpolation with  10.0 s   resolution took 0.8960 s
Interpolation with   5.0 min resolution took 0.1998 s
Interpolation with  30.0 min resolution took 0.1333 s

The local test failure is quite odd. I see it in the current astropy master as well. I don't know why, but I'm going to start by cleaning out all of my anaconda environments.

@mhvk

mhvk commented Nov 24, 2020

Copy link
Copy Markdown
Contributor

Well, hardly worth the 0.5% improvement in timing, but if anybody has very large arrays of SkyCoord, they will be thankful...

@mkbrewer

mkbrewer commented Nov 24, 2020

Copy link
Copy Markdown
Contributor

Just a little suggestion. You could call this icrs_to_observed() add an argument that takes either 'A' or 'H' in order to return either Alt/Az or HA/Dec and then create an icrs_to_altaz() function that calls icrs_to_observed() with the additional argument set to 'A'. In this way, this would be all set for the addition of an observed HA/Dec frame (see issue #10239). You could even add an icrs_to_hadec() function in anticipation of this, although that may be beyond the scope of this PR.

@mkbrewer

mkbrewer commented Nov 24, 2020

Copy link
Copy Markdown
Contributor

Of course, this applies to the reverse transform also.

@StuartLittlefair

StuartLittlefair commented Nov 24, 2020

Copy link
Copy Markdown
Contributor Author

Just a little suggestion. You could call this icrs_to_observed() add an argument that takes either 'A' or 'H' in order to return either Alt/Az or HA/Dec and then create an icrs_to_altaz() function that calls icrs_to_observed() with the additional argument set to 'A'. In this way, this would be all set for the addition of an observed HA/Dec frame (see issue #10239). You could even add an icrs_to_hadec() function in anticipation of this, although that may be beyond the scope of this PR.

This is a neat idea. I'll get to this tomorrow, but JWST deadline needs to take priority for now.

mhvk
mhvk approved these changes Nov 24, 2020
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good now! I also like @mkbrewer's idea, but would suggest doing that in a follow-up PR, as this one works well on its own (and doesn't hinder adding HADec after). So, OK to merge as far as I'm concerned.

@mhvk

mhvk commented Nov 24, 2020

Copy link
Copy Markdown
Contributor

Actually, this needs a changelog entry (or no-changelog-entry-needed label if we consider this just an implementation detail)

@mkbrewer

mkbrewer commented Nov 24, 2020

Copy link
Copy Markdown
Contributor

Just trying to kill two birds with one stone here.

@mhvk

mhvk commented Nov 24, 2020

Copy link
Copy Markdown
Contributor

Which is a good idea, but even though the code is, as you note, easy, writing the test cases and documentation will mean it will take a couple of hours, so might as well get this in...

@StuartLittlefair
StuartLittlefair merged commit 274be91 into astropy:master Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

coordinates Enhancement

Projects

None yet

Milestone

v4.3

Development

Successfully merging this pull request may close these issues.

Implement direct ICRS -> Observed transform

3 participants

@StuartLittlefair @mhvk @mkbrewer