Skip to content

Use astropy.coords in nrao module#67

Merged
keflavich merged 7 commits into
astropy:masterfrom
cdeil:nrao_coords
Apr 14, 2013
Merged

Use astropy.coords in nrao module#67
keflavich merged 7 commits into
astropy:masterfrom
cdeil:nrao_coords

Conversation

@cdeil

@cdeil cdeil commented Apr 13, 2013

Copy link
Copy Markdown
Member

I haven't tested this (my tests fail locally because of an independent issue) ... please review ... and let's see what travis-ci has to say.

@keflavich

Copy link
Copy Markdown
Contributor

This won't test against @astrofrog's .travis PR right now - I'm inclined to merge this, then ask Tom to merge/rebase against master, then see if we can get all the .travis builds working. Or, alternatively, we could merge the .travis PR (which seems fine) and rebase this against it. Which do you prefer?

@keflavich

Copy link
Copy Markdown
Contributor

I'll let both travis builds finish (a few hours) then make some decisions and see if we can't get both of these merged.

@cdeil

cdeil commented Apr 13, 2013

Copy link
Copy Markdown
Member Author

@keflavich I just saw #16 . You can close that one when you merge this.

@astrofrog

Copy link
Copy Markdown
Member

I think that you can merge #64 first, that way when this gets merged in it will get tested and we can then address any failures.

@astrofrog

Copy link
Copy Markdown
Member

@cdeil - would it be easy for you to rebase and push so we can re-test?

@keflavich

Copy link
Copy Markdown
Contributor

Merged #64, so rebase against master now?

@cdeil

cdeil commented Apr 13, 2013

Copy link
Copy Markdown
Member Author

Rebased.

@keflavich

Copy link
Copy Markdown
Contributor

I restarted the travis build; I think on a rebase, there's no travis build trigger

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is the following change OK?
I don't have the old coords package available right now for testing and there is no unit test for this.

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.

I think so:

In [3]: ra,dec = coords.Position((43.1,0.01),system='galactic').j2000()

In [4]: galactic = coord.GalacticCoordinates(43.1,0.01,unit=('deg','deg'))

In [5]: radec = galactic.fk5

In [10]: abs(radec.ra.degrees - ra) * 3600
Out[10]: 0.14393172129985032

In [11]: abs(radec.dec.degrees - dec) * 3600
Out[11]: 0.21757954918086853

so the answers differ by <1", which may be the limit of accuracy for one package or the other.

@cdeil

cdeil commented Apr 13, 2013

Copy link
Copy Markdown
Member Author

@keflavich Eventually there is a travis build when a rebase is pushed, but sometimes it doesn't start right away and it only shows up on the github UI once it's finished.

@cdeil

cdeil commented Apr 13, 2013

Copy link
Copy Markdown
Member Author

@keflavich @astrofrog Do you understand the travis test failures here ?

E.g. https://travis-ci.org/astropy/astroquery/jobs/6316304 shows the KeyError with threading.
And https://travis-ci.org/astropy/astroquery/jobs/6316302 shows a Python 3 string / unicode problem.

@keflavich

Copy link
Copy Markdown
Contributor

No, @astrofrog? I posted the same issue on #64, so I don't think it's an issue with this PR - I'm happy to merge this now.

@astrofrog

Copy link
Copy Markdown
Member

I'm just going to quickly diagnose the issue with python 3 string/unicode

@keflavich

Copy link
Copy Markdown
Contributor

OK, @astrofrog, merge when you're satisfied then

@astrofrog

Copy link
Copy Markdown
Member

The unicode/string issue is actually a big in the error display in astropy's master (and 0.2.1) but if I use a locally fixed version, building's @cdeil's branch gives:

Stdout:
You need the 'mechanize' module

Stderr:
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.3/lib/python3.3/site-packages/astropy/config/configuration.py", line 646, in generate_all_config_items
    for cfgitem in get_config_items(nm).values():
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.3/lib/python3.3/site-packages/astropy/config/configuration.py", line 533, in get_config_items
    __import__(packageormod)
  File "./astroquery/tests/test_ukidss.py", line 1, in <module>
    from astroquery import ukidss
  File "./astroquery/ukidss/__init__.py", line 1, in <module>
    from .ukidss import *
  File "./astroquery/ukidss/ukidss.py", line 11, in <module>
    import htmllib
ImportError: No module named 'htmllib'

which is the source of the error. Actually I see the same issue in master, so I guess this PR isn't the cause of the error.

@astrofrog

Copy link
Copy Markdown
Member

The threading error is actually a symptom of another failure - that there are warnings in the docs build. So if you can get rid of those warnings, the threading error will disappear.

@astrofrog

Copy link
Copy Markdown
Member

I think we should try and fix these before merging, but I'm out of time. Just for info, to get the proper error for the first case, you need to be running Python 3, and change

        msg = ('Generation of default configuration item failed! Stdout '
               'and stderr are shown below.\n'
               'Stdout:\n{stdout}\nStderr:\n{stderr}').decode('UTF-8')

to

        msg = ('Generation of default configuration item failed! Stdout '
               'and stderr are shown below.\n'
               'Stdout:\n{stdout}\nStderr:\n{stderr}')

in astropy/setup_helpers.py.

@keflavich

Copy link
Copy Markdown
Contributor

@astrofrog - have you promoted these to astropy issues?

I'd prefer to merge this, then have other issues pointing back to these problems, since this PR is not responsible for either of those bugs (I think #64 is the one actually responsible).

The doc errors also deserve their own issue.

All that said, I'll leave it open for now - I'm not really in a huge hurry, I just think this is not the right forum for those issues.

@astrofrog

Copy link
Copy Markdown
Member

@keflavich - just to clarify, the bugs in Astropy (which I think are both reported, will check tomorrow) are hiding two real issues in the package, but feel free to address them in a separate pull request. But you should be able to fix them here such that the tests pass, the bugs in Astropy are just in the actual errors themselves, not the fact an error is raised.

@keflavich

Copy link
Copy Markdown
Contributor

Since nothing in this PR breaks builds, but removing coords may fix other builds (got some SWIG errors in some builds because of this), I'm going to merge it. Further discussion of the errors @astrofrog noted should go in #70

keflavich added a commit that referenced this pull request Apr 14, 2013
Use astropy.coords in nrao module
@keflavich keflavich merged commit 17fe241 into astropy:master Apr 14, 2013
keflavich pushed a commit to keflavich/astroquery that referenced this pull request Jun 30, 2014
removed setuptools_bootstrap.py as it doesn't exist any more
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants