Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use astropy.coords in nrao module #67

Merged
merged 7 commits into from
Apr 14, 2013
Merged

Conversation

cdeil
Copy link
Member

@cdeil cdeil commented Apr 13, 2013

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
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
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
Copy link
Member Author

cdeil commented Apr 13, 2013

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

@astrofrog
Copy link
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
Member

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

@keflavich
Copy link
Contributor

Merged #64, so rebase against master now?

@cdeil
Copy link
Member Author

cdeil commented Apr 13, 2013

Rebased.

@keflavich
Copy link
Contributor

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

@@ -90,9 +84,15 @@ def get_nrao_image(lon, lat, system='galactic', epoch='J2000', size=1.0,
if band not in valid_bands:
raise ValueError("Invalid band. Valid bands are: %s" % valid_bands)

Copy link
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
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
Copy link
Member Author

cdeil commented Apr 13, 2013

@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
Copy link
Member Author

cdeil commented Apr 13, 2013

@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
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
Member

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

@keflavich
Copy link
Contributor

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

@astrofrog
Copy link
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
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
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
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
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
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.

None yet

3 participants