Add full set of celestial conversions; add some documentation #4

Merged
merged 24 commits into from Nov 27, 2012

Conversation

Projects
None yet
7 participants
Member

cdeil commented Nov 19, 2012

This is a first attempt at giving precise definitions for the input and various output sky definitions.

I don't know much about coordinates, so this definitely should be reviewed (or re-written) by a coordinate expert.

Once we have well-defined sky definitions I guess results should match almost within machine precision?

Member

cdeil commented Nov 23, 2012

@dsberry @astrofrog Should we add some more tests, e.g. for ICRS, SuperGalactic or other epochs of observation?
I think it doesn't matter if they're not is astropy yet, it'll be test-driven development.

Maybe we could take this diagram and add on test for each box?
http://www.astro.rug.nl/software/kapteyn/celestialbackground.html#composing-other-transformations

Should I give this a try with kapteyn and then we add for the other packages what is available there?

We should also make sure to resolve or document the differences we have at the moment based on @dsberry's detailed feedback on astropy-dev:
https://groups.google.com/forum/?fromgroups=#!topic/astropy-dev/tbcpMQ1rOcY
https://groups.google.com/forum/?fromgroups=#!topic/astropy-dev/2wATprLvzvo
https://groups.google.com/forum/?fromgroups=#!topic/astropy-dev/-ulzSY9qJKk

Contributor

dsberry commented Nov 23, 2012

I suppose since the purpose of the test suite is to test the features of
the astropy coordinates package, there is no real need to include tests of
features that are currently outside the scope of the coordinates package.
They could be added when and if the features become available. Having said
that, since the coordinates package does currently handle ICRS, it may make
sense to add that specific test.

David

On 23 November 2012 09:40, Christoph Deil notifications@github.com wrote:

@dsberry https://github.com/dsberry @astrofroghttps://github.com/astrofrogShould we add some more tests, e.g. for ICRS, SuperGalactic or other epochs
of observation?
I think it doesn't matter if they're not is astropy yet, it'll be
test-driven development.

Maybe we could take this diagram and add on test for each box?

http://www.astro.rug.nl/software/kapteyn/celestialbackground.html#composing-other-transformations

Should I give this a try with kapteyn and then we add for the other
packages what is available there?

We should also make sure to resolve or document the differences we have at
the moment based on @dsberry https://github.com/dsberry's detailed
feedback on astropy-dev:
https://groups.google.com/forum/?fromgroups=#!topic/astropy-dev/tbcpMQ1rOcY
https://groups.google.com/forum/?fromgroups=#!topic/astropy-dev/2wATprLvzvo
https://groups.google.com/forum/?fromgroups=#!topic/astropy-dev/-ulzSY9qJKk


Reply to this email directly or view it on GitHubhttps://github.com/astropy/coordinates-benchmark/pull/4#issuecomment-10654265.

Owner

astrofrog commented Nov 23, 2012

@cdeil - I think first we should start adding the remaining tests that we can do with astropy , e.g. FK4 -> FK5, FK4 -> Galactic. We should re-name all the output files e.g. coords_fk5_to_fk4, etc. giving the original and final system. We should also change j2000 and b1950 to fk5 and fk4 since we should refer to them by the system, not the equinox. Would you like to do this? I think for now, we need:

FK5 -> FK4
FK4 -> FK5
FK5 -> ICRS
ICRS -> FK5
FK4 -> Galactic
Galactic -> FK4
FK5 -> Galactic
Galactic -> FK5

Note that Astropy does not currently include the e-terms in the FK4 transformation, which is why we are offset by ~0.3". I am working on a pull request to astropy.coordinates that fixes this.

Member

cdeil commented Nov 23, 2012

I'll do this tomorrow.

Member

cdeil commented Nov 23, 2012

@astrofrog Can you do IDL, I've never used it and don't want to.

Owner

astrofrog commented Nov 23, 2012

That's the spirit ;-) I've never really used it either, but I have access to a machine with IDL, so I'll see to that part.

Owner

astrofrog commented Nov 25, 2012

@cdeil - when you get started on implementing the different conversions, could you put a note here? If you don't have time to do it over the next few days, I may do it, as I need the B1950 -> J2000 tests for some of the fixes I'm preparing for astropy.coordinates.

Member

cdeil commented Nov 25, 2012

I've started working on this. I'll make a PR tonight.

Owner

astrofrog commented Nov 25, 2012

Great, thanks!

Member

cdeil commented Nov 25, 2012

@astrofrog I've added a script run_benchmark.py in the top-level folder to avoid boilerplate code for each tool and to make it possible to time them later.
So far I've modified kapteyn and astropy for this new format and added their output, so you can now start to check astropy against kapteyn.
I'll add the other tools tomorrow.

Owner

astrofrog commented Nov 25, 2012

Awesome, thanks for all your work on this! I will try out your fork.

Member

cdeil commented Nov 26, 2012

@astrofrog I'm just converting tpm to the new scheme and noticed that I get different results at the 1e-6 level for FK5 to ecliptic on my machine. What version of astrolib.coords are you using?

I have coords-0.37.tar.gz

For FK4 and Galactic I get identical output files to what you committed.

wlan-3-154:tpm deil$ mv coords_ecliptic.txt old_coords_ecliptic.txt

wlan-3-154:tpm deil$ python convert.py 

wlan-3-154:tpm deil$ head *ecliptic*.txt
==> coords_ecliptic.txt <==
 353.813134485264698   43.074512920941565
 111.339967737481587    4.773239402161311
  70.502355462059086   15.156361860053401
  82.028989371229414   47.639312640189679
 175.028829753960821   53.165696442721313
 207.595699492095946   24.554353430690330
 337.479252142436280  -19.643831188650484
 232.242935099295067   21.137623710423636
 269.543463014886811   31.356642546652409
 171.484392219117154   75.725410878729065

==> old_coords_ecliptic.txt <==
 353.813137191203566   43.074513234681895
 111.339967649021602    4.773236690593438
  70.502355725257047   15.156359115829238
  82.028989813943767   47.639309757152049
 175.028825882000376   53.165696190455812
 207.595698313371742   24.554354779227513
 337.479251182550286  -19.643830073622265
 232.242934410121507   21.137626012031067
 269.543463000751899   31.356645457724252
 171.484380903108729   75.725410447647022

wlan-3-154:tpm deil$ ipython
In [2]: import numpy as np

In [3]: old = np.loadtxt('old_coords_ecliptic.txt')

In [4]: new = np.loadtxt('coords_ecliptic.txt')

In [5]: new - old
Out[5]: 
array([[ -2.70593887e-06,  -3.13740330e-07],
       [  8.84599842e-08,   2.71156787e-06],
       [ -2.63197961e-07,   2.74422416e-06],
       ..., 
       [ -1.99689794e-08,  -2.91093065e-06],
       [  1.42366170e-06,   2.54488909e-06],
       [ -1.25292388e-07,   2.72659830e-06]])
Member

cdeil commented Nov 26, 2012

@astrofrog When I run tpm/convert.py unmodified, I find for FK5 -> Ecliptic an agreement with the other tools at the level of 1 milli-arcsec, whereas the current tpm/coords_ecliptic.txt in the repo differs by 5 arcsec from all the other tools. I assume this is simply an old version and add the current result to this PR now, which in the new scheme is called tpm/fk5_to_ecliptic.txt.

cdeil added some commits Nov 26, 2012

@cdeil cdeil Add celestial conversions for all systems for tpm.
Update summary table accordingly.
0e5bb22
@cdeil cdeil Minor cleanup 5828ff9
@cdeil cdeil Rename tpm to astrolib.
We want to add pytpm and this rename will avoid confusion.
a9100f0
Owner

astrofrog commented Nov 26, 2012

@cdeil - it might have been an old version, so just ignore the discrepancy! I'll re-run all the tests locally to see if the results change.

Member

cdeil commented Nov 26, 2012

I've started a summary page for the different coordinate tools in Python:
https://github.com/cdeil/coordinates-benchmark/blob/1c894705a862c5f94b2e6aebe815062f86bf3c32/docs/Tools.rst

I would like to mention the license of each package, especially if it is BSD and thus astropy compatible, but for some tools I couldn't figure it out quickly.
This licensing stuff is important should we decide to look at existing libraries / wrappers for astropy in the future.

@dsberry What is the license of pyast and AST? For pyast can you add this info in the README and at http://dsberry.github.com/starlink/pyast.html ?

@scottransom What is the license of pyslalib and SLALIB? For pyslalib can you please add this info to the README?

@brandon-rhodes What is the license of pyephem and the xephem code it contains? Is pyephem dual-licensed with GPL and LGPL? Can you please make it more clear by mentioning the license in the README and at http://rhodesmill.org/pyephem/ ?

Does anyone know what the TPM license is?

Contributor

dsberry commented Nov 26, 2012

On 26 November 2012 16:31, Christoph Deil notifications@github.com wrote:

I've started a summary page for the different coordinate tools in Python:

https://github.com/cdeil/coordinates-benchmark/blob/1c894705a862c5f94b2e6aebe815062f86bf3c32/docs/Tools.rst

I would like to mention the license of each package, especially if it is
BSD and thus astropy compatible, but for some tools I couldn't figure it
out quickly.
This licensing stuff is important should we decide to look at existing
libraries / wrappers for astropy in the future.

@dsberry https://github.com/dsberry What is the license of pyast and AST?
For pyast can you add this info in the README and at
http://dsberry.github.com/starlink/pyast.html ?

https://github.com/scottransom

AST and pyast are currently GPL.

David

@scottransom https://github.com/scottransom What is the license of
pyslalib and SLALIB? For pyslalib can you please add this info to the
README?

@brandon-rhodes https://github.com/brandon-rhodes What is the license
of pyephem and the xephem code it contains? Is pyephem dual-licensed with
GPL and LGPL? Can you please make it more clear by mentioning the license
in the README and at http://rhodesmill.org/pyephem/ ?

Does anyone know what the TPM license is?


Reply to this email directly or view it on GitHubhttps://github.com/astropy/coordinates-benchmark/pull/4#issuecomment-10722204.

Contributor

scottransom commented Nov 26, 2012

On 11/26/2012 11:31 AM, Christoph Deil wrote:

I've started a summary page for the different coordinate tools in Python:
https://github.com/cdeil/coordinates-benchmark/blob/1c894705a862c5f94b2e6aebe815062f86bf3c32/docs/Tools.rst

I would like to mention the license of each package, especially if it is
BSD and thus |astropy| compatible, but for some tools I couldn't figure
it out quickly.
This licensing stuff is important should we decide to look at existing
libraries / wrappers for |astropy| in the future.

@dsberry https://github.com/dsberry What is the license of |pyast| and
|AST|? For |pyast| can you add this info in the README and at
http://dsberry.github.com/starlink/pyast.html ?

@scottransom https://github.com/scottransom What is the license of
|pyslalib| and |SLALIB|? For |pyslalib| can you please add this info to
the README?

Hi Christoph et al.

This is already in the README for pyslalib. The license is GPL (as the
fortran SLALIB code is GPL'd).

Scott

Scott M. Ransom Address: NRAO
Phone: (434) 296-0320 520 Edgemont Rd.
email: sransom@nrao.edu Charlottesville, VA 22903 USA
GPG Fingerprint: 06A9 9553 78BE 16DB 407B FFCA 9BFA B6FF FFD3 2989

Member

cdeil commented Nov 26, 2012

@scottransom Thanks for the info. I was only searching for "license" in my browser and you have this wording:
"The version of SLALIB included here is 2.5-4 (with several additional tweaks) and is released under the GPL."

Member

cdeil commented Nov 26, 2012

@astrofrog Would it be OK to merge this now and then continue in separate issues / PRs.

@dsberry Could you please have a look at the table where I tried to specify exactly all relevant parameters for the input and output "sky definitions" we are comparing here?
https://github.com/astropy/coordinates-benchmark/pull/4/files#L31R55
I'm thinking we could maybe declare the pyast results as the reference values, but then we need to clearly specify exactly what the systems are so that we have a chance to identify the differences we see wrt. the other tools.

Now all results for all tools and conversion agree within 1 arcsec, but there's still quite a few cases where results disagree by more than a milli-arcsecond:
https://raw.github.com/cdeil/coordinates-benchmark/6dd119c686f50f64dcd2a206553b3ce3be9ae12f/summary.txt

Owner

astrofrog commented Nov 26, 2012

@cdeil - feel free to merge! does it look like you have adequate permissions?

@cdeil cdeil added a commit that referenced this pull request Nov 27, 2012

@cdeil cdeil Merge pull request #4 from cdeil/specification
Add full set of celestial conversions; add some documentation
c0e6eab

@cdeil cdeil merged commit c0e6eab into astropy:master Nov 27, 2012

Owner

astrofrog commented Nov 27, 2012

@cdeil - thanks for all your work on this! I agree it might make sense to just declare one of the tools the reference (e.g. AST)

Contributor

dsberry commented Nov 27, 2012

On 26 November 2012 21:56, Christoph Deil notifications@github.com wrote:

@astrofrog https://github.com/astrofrog Would it be OK to merge this
now and then continue in separate issues / PRs.

@dsberry https://github.com/dsberry Could you please have a look at the
table where I tried to specify exactly all relevant parameters for the
input and output "sky definitions" we are comparing here?
https://github.com/astropy/coordinates-benchmark/pull/4/files#L31R55
I'm thinking we could maybe declare the pyast results as the reference
values, but then we need to clearly specify exactly what the systems are so
that we have a chance to identify the differences we see wrt. the other
tools.

  1. In the following line:
  • Reference system (fk4, fk4_no_e, fk5, icrs, j2000, dynj2000)

it is not made clear that "j2000" and "dynj2000" are actually just synonyms
for the same thing. Maybe you should change "j2000, dynj2000" to
"j2000/dynj2000".

  1. In the table, for the "Epoch of observation", you probably want to use
    the same value (J2000) for all systems, at least initially. Having said
    that, you have a problem if any of the coordinate packages do not allow the
    epoch of observation to be specified. For instance, I'm not sure what the
    current implementation of astropy coords assumes about the epoch of
    observation. I understand Erik has agreed that an ObsTime property is
    needed, but I presume he hasn't yet added it.

  2. For the Equinox column, the equinox does not enter into the definition
    of icrs and galactic, so maybe "---" would be a better entry than "???".

  3. Galactic coordinates are defined by (ra,dec) positions specified in the
    FK4 system, but since there is no other alternative (i.e. we are not free
    to change this definition), the reference system is not a free parameter
    and so maybe should be set to "---".

  4. For ecliptic, my understanding is that no reference system is needed. I
    see that Kapteyn allows different reference systems to be associated with
    ecliptic coords, but looking at the code (function MatrixEq2Ecl in
    celestial.py) the only use it makes of the reference system is to decide if
    the supplied epoch is interpreted as Besselian or Julian. This is done
    differently in AST - you specify B or J when storing the epoch. So if you
    always make sure that that the Equinox and Epoch of Observation always
    include a B or J explicitly, the reference system should not be relevant to
    ecliptic.

  5. A further development could be to add more tag names for other
    combinations, like "fk5_J2010_J2012.34" meaning equatorial fk5 with respect
    to equinox of J2010, observed at J2012.34. Converting to and from systems
    with non-default values for equinox and epoch should test more bits of the
    code. Although of course the total number of tests could become difficult
    to handle.

  6. When horizon coordinates are added, you will need to add columns for the
    observer's longitude, latitude and altitude, and the epoch of observation
    will be required to greater accuracy.

David

Member

cdeil commented Nov 27, 2012

@dsberry Thank you very much for your notes, here's the new version:
https://github.com/astropy/coordinates-benchmark/blob/master/docs/Specification.rst

Owner

eteq commented Nov 27, 2012

@dsberry - to answer your question/comment here: the current astropy.coordinates assumes the epoch and equinox are the same (with attribute name epoch), but as you say, that will soon change to two equinox and obstime attributes.

Member

cdeil commented Jan 21, 2013

@brandon-rhodes: At http://pypi.python.org/pypi/pyephem I can see that pyephem is LGPL-licensed. What about the xephem parts of the code, are they proprietary like xephem or did the author re-license them as LGPL?
( I'm trying to assemble an overview of licenses and features of existing astronomy coordinate packages here.)

Contributor

brandon-rhodes commented Jan 24, 2013

Good question — and in fact, the Debian project was so thorough in their questioning before they would allow someone to add PyEphem as a Debian package that they discovered contributors that even I did not know about! The result of the questions they asked resulted in the COPYING file, which should give you the whole story (the upshot of which is, everything in the entire distribution is LGPL):

https://github.com/brandon-rhodes/pyephem/blob/master/COPYING

@cdeil cdeil added a commit to cdeil/coordinates-benchmark that referenced this pull request Feb 24, 2014

@cdeil cdeil Add license info for pyephem
This info was given by Brandon Rhodes here:
astropy#4 (comment)
3940b6b
Contributor

timj commented on 6dd119c Nov 18, 2014

@cdeil The AST library is now LGPL. Maybe @dsberry can confirm with a pull request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment