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

WIP: VAMDC-cdms interface #658

Merged
merged 12 commits into from
Oct 22, 2016
Merged

WIP: VAMDC-cdms interface #658

merged 12 commits into from
Oct 22, 2016

Conversation

keflavich
Copy link
Contributor

See #618. The goal of this PR is to include an API similar to the Splatalogue
API for accessing information from the CDMS database.

cc @vilhelmp

@keflavich
Copy link
Contributor Author

I now use this regularly in some code. @vilhelmp, any chance you could review it? Otherwise, @bispocz, any objections to merging this in its current state? It is a bit sparse on functionality, but it does something.

@keflavich
Copy link
Contributor Author

Wait, nevermind - can't merge this until I add some tests! And it requires vamdclib.

@bsipocz
Copy link
Member

bsipocz commented Aug 2, 2016

Since there are quite a few modules that are far from being complete, I think it's fine to merge this.
Although a rebase, to get rid of the irrelevant merge commits, and some tests would be nice.

@bsipocz
Copy link
Member

bsipocz commented Aug 2, 2016

(Commented on the wrong PR first) There isn't much info about the install and deps on the docs pages of vamdclib, so I'm just blindly making suggestions here:
We could either try to build a conda package for it, or install it separately in .travis.yml. Former is better if it takes a long time to install, and/or there are other packages that may use it, latter seems simpler.

@vilhelmp
Copy link
Contributor

vilhelmp commented Aug 3, 2016

Would love to test it out. I just switched to using Anaconda Python though. Have to figure out how to test a separate build. Virtualenv I guess?
@keflavich @bsipocz

@keflavich
Copy link
Contributor Author

you can make a separate conda environment, but I usually just use python setup.py develop and switch between branches to test new features

@keflavich
Copy link
Contributor Author

I'm going to link .travis.yml against my personal fork of vamdclib because I've incorporated package-template into it. Otherwise, my branch should match upstream, and this puts the burden on me to keep it up-to-date. I'll try to convince the vamdc maintainer to incorporate package-template though...

@bsipocz
Copy link
Member

bsipocz commented Sep 1, 2016

@keflavich - I wonder whether you have the vamdclib from upstream? It looks like that cpe/vamdclib has only one fork, yours?

@keflavich
Copy link
Contributor Author

@bsipocz this is a confusion situation. There are TWO identical upstreams: cpe/vamdclib and VAMDC/vamdclib. I have forks of both... I think they were created independently but should not be independent. I filed a request with github support to try to fix the situation, but I'm not sure there's anything they can do about it. In any case, I just submitted a PR to incorporate package-template

@bsipocz
Copy link
Member

bsipocz commented Sep 1, 2016

Sounds good.

@keflavich
Copy link
Contributor Author

It seems to be nontrivial to create a non-remote-dependent vamdclib request. I may elect for remote-only tests for this module. It will result in more likely breakage, but I'm not sure there's anything to be done about that. It's a fairly niche module anyway.

@bsipocz
Copy link
Member

bsipocz commented Sep 1, 2016

Sure. In that case I would suggest to add a second line in travis where we run the tests that runs the remote for only for this module.

@keflavich
Copy link
Contributor Author

@bsipocz do you know what's going on with https://travis-ci.org/astropy/astroquery/jobs/169277855#L1303? I don't understand why it's complaining about not having a VAMDC API page:

/home/travis/build/astropy/astroquery/docs/vamdc/vamdc.rst:49: WARNING: toctree references unknown document u'api/astroquery.vamdc.Vamdc'

since that should be generated.

@bsipocz
Copy link
Member

bsipocz commented Oct 21, 2016

@keflavich - This is weird, locally there are no warnings, but I've seen similar warnings before. Let me dig a bit deeper.

@bsipocz
Copy link
Member

bsipocz commented Oct 21, 2016

I've restarted travis to see whether it was just a fluke. Also could you please rebase to get rid of the merge commits?

try:
Vamdc = VamdcClass()
except ImportError:
log.warn("vamdclib could not be imported; the vamdc astroquery module will not work")
Copy link
Member

Choose a reason for hiding this comment

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

pls use log.warning as warn is deprecated

@bsipocz
Copy link
Member

bsipocz commented Oct 21, 2016

@keflavich - so the sphinx warning indeed went away, and you could get rid of the remaining failures with the warn-->warning change.

@keflavich
Copy link
Contributor Author

@bsipocz I can rebase, but is there any particular reason to? I don't mind having merge commits around so much.

Thanks for catching warn vs warning... I knew one was deprecated but can never remember which.

@bsipocz
Copy link
Member

bsipocz commented Oct 21, 2016

No strong reason, I usually prefer to have a cleaner branch structure without the merges as it's slightly easier to see what going on.

@keflavich
Copy link
Contributor Author

keflavich commented Oct 22, 2016

OK, rebased and I think it's ready to merge. Anything to add @bsipocz?

(besides tests, but this one is going to be an experimental / test-free module for the foreseeable future b/c of its dependencies)

@bsipocz
Copy link
Member

bsipocz commented Oct 22, 2016

@keflavich - Sure, let's get this merged.

@keflavich keflavich merged commit 3f900d3 into astropy:master Oct 22, 2016
@keflavich keflavich deleted the vamdc branch October 22, 2016 17:08
@vilhelmp
Copy link
Contributor

I quickly made a small function to query CDMS through their web interface. https://github.com/vilhelmp/cdmspy
Don't know if there's any use in getting it into astroquery?

@keflavich
Copy link
Contributor Author

@vilhelmp yeah, even though it's somewhat redundant with vamdc, I think having a direct interface to their web page is valuable. vamdclib doesn't fall within the astropy ecosystem at all, and it doesn't follow all the 'normal' python conventions, so I don't know how much we can rely on it long term. Please open a PR!

@bsipocz bsipocz modified the milestone: 0.3.4 Nov 21, 2016
@bsipocz bsipocz mentioned this pull request Jan 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants