VO Client and Server for Cone Search #552
Conversation
|
@mdboom might want to look at the votable stuff that I modified. They should not affect votable functionality itself. I needed to make those changes for them to work with my cone search stuff. |
astropy/io/votable/table.py
Outdated
There was a problem hiding this comment.
The docstring should be used to describe what the function does. This information is helpful, but should probably be in a comment following the docstring.
The docstring could be something like:
Resets all of the vo warning state so that warnings that have already been emitted will be emitted again. This is used, for example, by `validate` which must emit all warnings each time it is called.
|
This is great -- my comments are mostly niggly details, but overall I think this is really good and a real improvement over what I had done years ago. |
|
One other question: for the server-side code that checks the services and generates JSON files, is there a simple script that could be run to add it to a cron job? I think that's what most people would want who will be installing this on a server. Maybe we should add something to do this in I'm also noticing that "server" is slightly the wrong name, since it's really just a tool that is run periodically to generate static files that are the served by a web server. I don't know if it matters enough to change the name, and it's certainly a clear separation -- just thought I'd point it out. |
|
I agree with @mdboom on the naming issue. I don't have a better suggestion at the moment but I'll think on it; "server" is not too accurate though (unless we want to actually add a simple server too, which could be done with low effort). |
|
"validator"? "utils"? |
astropy/utils/misc.py
Outdated
There was a problem hiding this comment.
This is fine. But Python 3.2 has a concurrent.futures module (http://docs.python.org/dev/library/concurrent.futures.html) for this purpose. There is a very clean backport for >=2.5 available at http://pypi.python.org/pypi/futures (I'm not sure, but, I think this was the original version of the package before it was added to the stdlib). It might be worth adding to Astropy if we have a need for this sort of thing, now that there's a "standard" implementation.
There was a problem hiding this comment.
I'll add, an additional advantage to this approach is that it supports thread and process-based executors with the same API; the cone search executor could optionally use either one of them.
There was a problem hiding this comment.
If AstroPy allows pypi/futures as optional dependency, I can replace Future class with that, no problem.
There was a problem hiding this comment.
Just something to consider; maybe I'll add an issue for it. For the purposes of this PR this implementation is perfectly fine. If/when we include concurrent.futures, we can go back to this and swap this out for it.
|
General question: What is the workflow to apply suggested changes to existing pull request? I am not very familiar with how GitHub works. Do I create a new branch? Or modify the same branch and re-submit Pull Request? |
There was a problem hiding this comment.
Ditto here and below on the missing object base class. It really doesn't likely make a difference but it can't hurt either. Old-style classes do have some differences and I occasionally get confused when I encounter one in the wild.
…urking in VAO registry. As the registry does change over time, there is no guarantee things will not break again if user decides to validate the *entire* VAO registry.
|
@pllim - to get the tests to pass, you need to move the Numpy import in astropy.utils.misc to be inside the function that uses it, because |
|
@astrofrog , thanks, Travis has passed! |
|
Great! So I think this can be merged now since I think @perrygreenfield gave the go-ahead. @eteq - do you agree? |
|
I think so! Let's get this in there. 😉 |
|
@pllim: Where do you find this stuff? |
👍 (Also on the merge, but probably at least as much the dance...) |
VO Client and Server for Cone Search
|
Hooray! |
|
Oh and @pllim or @iguananaut - I didn't want to delay the merge for this, but perhaps |
|
@pllim - thank you for this great contribution! :) |
|
Yes, there needs to be documentation of this in CHANGES.rst and the New for version 3.0 page. |
…structions Updated installation instructions
6 participants
As per #422