Skip to content

Conversation

@vilhelmp
Copy link
Contributor

Rewrote the NASA ADS search I had lying around for inclusion into astroquery. Instead of parsing the HTML results with Beautiful Soup, I request the results in XML, and use the xml.dom.minidom inspector to parse the results.

Simple search:

    import astroquery.nasa_ads as ads
    result  = ads.ADS.query_simple('^Ginsburg')
    # result is now a Astropy.Table table. eg:
    result['authors'] # to see the list of lists of authors

However, print result raises an error, I think it's because of the encoding, in the function astroquery.nasa_ads.utils.get_data_from_xml the parsing of the individual elements happens (.encode("utf-8") tries to encode the text field into utf-8).

@keflavich
Copy link
Contributor

For tables, you can use the function astropy.table.Table.convert_bytestring_to_unicode to fix some unicode issues, perhaps - I can't promise this is relevant to the problem you've noted, I'm just trying to pass on something I recently learned.

Thanks for the PR. I'll try to review it in more detail when I'm back from travel

@vilhelmp
Copy link
Contributor Author

I just discovered this: https://github.com/andycasey/ads

@keflavich
Copy link
Contributor

It is nice but we need an API key to use that. Maybe we can get one for
astroquery? I think a python interface to the simpler, old ADS form is
still useful

On Tue, Mar 17, 2015 at 11:18 AM, Magnus Vilhelm Persson <
notifications@github.com> wrote:

I just discovered this: https://github.com/andycasey/ads


Reply to this email directly or view it on GitHub
#499 (comment).

Adam

@keflavich
Copy link
Contributor

Can you rebase or merge against master? we fixed the astropy build issue in #503

@vilhelmp
Copy link
Contributor Author

Yeah I don't know what I just did. I tried to rebase again, before adding the doc to index.rst. Got an error, that there were conflicts, from the first commit. Then I thought, I clone my fork again locally (to a new destination), and then work on that instead, that seem to have screwed up things on my machine, now I don't know what to do.

@keflavich
Copy link
Contributor

I'll investigate tomorrow.

@keflavich
Copy link
Contributor

@vilhelmp if you want some live help dealing with the git-related stuff, join gitter (https://gitter.im/astropy/astropy) and I can help walk you through the issues. Re-cloning should never be necessary, though I did it a few times before I understood rebasing.

As for the error, it's still a simple one:
https://travis-ci.org/astropy/astroquery/jobs/58165454#L1473
just need to add the NASA ADS entry in the index.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.33%) to 65.88% when pulling 1a659ec on vilhelmp:master into 909fc9a on astropy:master.

@vilhelmp
Copy link
Contributor Author

I get some errors in other modules, I think, probably because I updated my fork from upstream, and then pushed my changes. Since I am organizing a Software Carpentry boot camp today and tomorrow, I don't have time during daytime for gitter.

@keflavich
Copy link
Contributor

Travis is passing. Coverage is complaining because you don't have tests, but that isn't a blocker - tests would be really great, but I'm willing to incorporate un-locally-tested tools for the time being at least. So, is this ready to merge, from your side?

@keflavich
Copy link
Contributor

Actually, on further inspection, we need some docs and at least a remote test/example...

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.33%) to 65.88% when pulling 9f3e83c on vilhelmp:master into 8540277 on astropy:master.

Copy link
Contributor

Choose a reason for hiding this comment

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

this stuff should be in an indented code block:

.. code-block:: python

    from astro....

@keflavich
Copy link
Contributor

the failure is just sphinx

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 66.21% when pulling bd34224 on vilhelmp:master into 8540277 on astropy:master.

@keflavich
Copy link
Contributor

you're green now! ready to merge?

Copy link
Contributor

Choose a reason for hiding this comment

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

all these commented out things should be removed

@vilhelmp
Copy link
Contributor Author

Think I fixed everything!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) to 66.2% when pulling 2aae04f on vilhelmp:master into 8540277 on astropy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) to 66.2% when pulling 2aae04f on vilhelmp:master into 8540277 on astropy:master.

keflavich added a commit that referenced this pull request Apr 24, 2015
Search provider - NASA ADS
@keflavich keflavich merged commit b9c6ae6 into astropy:master Apr 24, 2015
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