Can you add this information:

• what type of objects are returned by these functions?
• what errors should be thrown if queries fail?
• how should timeouts be handled?

I need to dig out my notes on astropy.surveys from the coordination meeting - there was a preliminary API discussion about this (including asynchronous access).

To clarify, I'm thinking that each tool will have classes for querying, but by default the user will only be exposed to instances of the classes. i.e.:

from astroquery import fermi
result = fermi.FermiLAT_Query('object')

But if a user wanted multiple parallel queries with different parameters for whatever reason, they could do:

from astroquery.fermi.fermi import FermiLAT_QueryClass
newFermiQuery = FermiLAT_QueryClass()
newFermiQuery.request_url = 'http://....'

@keflavich How about a function fermi_lat_query instead of a class FermiLAT_Query?

Also see @eteq's comments on module organization:
astropy/astropy#493 (comment)

Basically astroquery does it wrong everywhere at the moment, i.e. it has astroquery/simbad/simbad.py but it should be either astroquery/simbad/core.py or if there's only one file, then astroquery/simbad.py.

Maybe we can do all the renaming now?

Argh, yes, we should probably do all the simbad.py -> core.py renaming - let's make a new PR for that (you want to take the lead?).

As for class vs. function, though - I wanted classes so that we could associate function-specific things (i.e., request URL, regexp for extracting links) with the "function" that would be using them. It allows for more flexibility in the future.

Also, as shown in this PR, we can have a parent class with a timeout parameter, which should be general for all class/functions.

In your example result = fermi.FermiLAT_Query('object') would result be a FermiLAT_Query object?

Consider this:

class A(object):
    def __init__(self):
        return 43
a = A()
# TypeError: __init__() should return None, not 'int'

and this:

class A(object):
    def __call__(self):
        return 42
a = A()
print type(a), a
# <class '__main__.A'> <__main__.A object at 0x10f4efc10>

i.e. you can't make a query class return results like tables or images or filenames or ...

That's why I proposed to use a function instead of a class.

It would be more like the latter:

class A_class(object):
     attribute = "hello"
     def __call__(self, arg):
          print arg, self.attribute

A = A_class()
result = A('m31')

is my intent: the class instances would be declared in the files. The real point is basically to have functions, but functions which can do this:

fermilatquery.timeout = 5
result=fermilatquery('m31')

Of course, the same functionality could be achieved with keyword arguments:

def query(object, url='http://....', timeout=5):

which might be preferable; arguments either way?

If I understand correctly in your example above you meant

result = fermi.FermiLAT_Query()('object')

instead of

result = fermi.FermiLAT_Query('object')

I think I don't like that __call__ does the actual query, I'd prefer if that is done by an SomeQuery.execute() or SomeService.query() method (the difference is just naming).

I'm not sure what class API, I think the choices are:

• parameters set via __init__ or execute or mixed?
• execute() call returns results or stores (caches) it in the query class.

But I think that every query should have a function wrapper which basically does:

def query_some_service(parameters):
    service = SomeService(parameters)
    return service.execute()

or

def query_some_service(parameters):
    service = SomeService()
    return service.execute(parameters)

@keflavich I think it would help if you write down your preferred solution in the API document and then illustrate it with a simple and complex example (login, multiple parallel queries).
This is basically there already, but parts are under a section "Present Implementations (April 2013)" and it's not clear that this is what we propose. Present implementation shouldn't appear at all in this document, I think. There's not so much code and it's easy to refactor everything to whatever we like.

@cdeil I like your ideas; why not separate the wrapper function from the class? However, I also think __call__ should call self.execute. See the latest updates. Still need to add other features, though....

A question: is the intent to allow some sort of "magic" coordinates parsing when you need to give coordinates? Or will it always be a Coordinates object that is passed in? I would favor the latter approach.

I am strongly in favor of "magic" for the wrapper functions: as an end
user, I really don't want to think about it as:

import coordinates as coord
from astroquery import tool

tool.query(coord.Coordinates('....'),...)

But rather:

from astroquery import tool

tool.query('ra dec')
tool.query('m31')

However, the classes should probably require a Coordinates object or an
object name.

The real trouble with requiring a Coordinate object at any stage, though,
is that it would require pre-parsing of the object names into coordinates,
which is something I don't think astroquery should do (because other
services, simbad etc, do that, and perhaps their behavior is different for
object names).

@eteq, can you identify reasons to require a Coordinates object? Perhaps
I'm missing an important point, or perhaps we can address them some other
way.

Ahh, In the name case, I definitely agree. tool.query('m31') should work without any need for a coordinate object of any sort.

I was thinking of the case of tool.query(coord, ...) where coord is an arbitrary coordinate, not an object name. In that case, I was saying we shouldn't support tool.query('1.2 3.5', ...) for the same reason we are trying to avoid that in coordinates: it's ambiguous what the user wants, and we should refuse the temptation to guess.

I more middling case is tool.query('1h23m12s +32d21m43.4s', ...), where the string is unambiguous. The main concern there is that the user is not specifying the system. What if the user thinks they are giving ICRS coordinates, but the tool actually accepts Galactic coordinates? On the other hand, doing tool.query(ICRS('1h23m12s +32d21m43.4s'), ...) resolves that ambiguity - you just have the query function internally always convert to galactic before actually sending it on to the remote server, and then the user can provide any kind of coordinate system.

This is a good point... I think for systems where there's a possibility that the remote server would try to parse an ambiguous coordinate like 1.5 3.5 we should have an explicit default system in place that matches the default system on the remote server.

I think it would be best to allow any service to take Coordinates objects, but in no case require it. So, there would be functions like this:

def tool(coord, system='fk5'):
      ....

that would ignore the system keyword if a Coordinates object was given, but would parse 1.5 3.5 as fk5 if passed in that way. Alternatively, we could just pass 1.5 3.5 into the Coordinates object and let it raise an exception for ambiguous coordinates - that is what it should do, right?

Yes, the plan is now to support something just like that (@keflavich's approach above using **kwargs). It's not in in yet, but the intent is to get that working by v0.3. I was originally thinking something like this (with kwargs), but the reason why I'm thinking tool(FK4(...)) is better is because the tool might have a bunch of different parameters (even two sets of coordinates), and then you have to figure out some way to map the kwargs onto each coordinate. Or you can have a scheme like tool(coord1, coord1kwargs, coord2, coord2kwargs, ...). But then why not just use the coordinate initializers, instead?

More generally, I'm rather worried about having astroquery make more assumptions (e.g., not requiring units). I think it will be more confusing to users if astroquery makes different assumptions than coordinates, because they'll forget which one applies where. And coordinates are coordinates, regardless of what is consuming them. (That's even moreso if different tools use different interfaces, but it sounds like you already agree that should be as internally consistent as possible.)

So if you really think there are some problems with how coordinates are specified (e.g. the degrees thing), then I think it'd be better all around to instead have that discussion in coordinates (although I couldn't resist discussing it a little at the bottom of this comment). Feel free to use "this would be better for astroquery" as an argument there. One of the main points is that we want an interface that can be used directly by affiliated packages, after all.

Also, 👍 on the parse_name suggestion from @keflavich's second comment - that will definitely help keep it all both more consistent and more readable.

<soapbox> regarding specifying units: I agree it is slightly more verbose, but that's good in my view. I bet if I go out and walk down the hallway of my building and ask everyone if they've been bitten by the hr/deg ambiguity in places like astrolib or some other IDL/iraf/whatever function before, I bet the answer will be yes for at least 50% of people. I agree it is not "typical", but we don't want to just re-implement the same thing people have done before - we want to do better. </soapbox>

I'm disappointed that github doesn't support the <soapbox> tag yet.

What you're saying all makes sense. It does mean that a minimal session must be

from astropy import coordinates
from astroquery import tool

result = tool(coordinates.FK5(ra,dec,units='deg,deg'))

I guess that's not really a problem.

I can't really justify it except by laziness, but I would really like a magic_parse_coordinates function.

Yeah, I tried :soapbox: first and was very sad to see it missing :)

It's certainly possible to revisit this topic later, because the magic part could easily be implemented as a layer on top of the part that uses only Coordinate and Quantity or similar objects. If we find people constantly asking on the mailing list "How do I use these stupid object things? I hate this!" then you'll be proven right :)

Related to my comment above, one can make a method behave like both a static or an instance method:

import functools


class static_or_instance(object):
    def __init__(self, func):
        self.func = func

    def __get__(self, instance, owner):
        return functools.partial(self.func, instance)


class UKIDSS(object):

    @static_or_instance
    def query(self):
        if self is None:
            print("Calling query as static method")
        else:
            print("Calling query as instance method")

which means the user can do:

from astroquery import UKIDSS
UKIDSS.query()

for simple queries, or

from astroquery import UKIDSS
u = UKIDSS()
u.query()

if e.g. log-in is required. This then avoids the need to duplicate all methods as wrapper functions.

(one can do something similar with class vs instance methods if the class is needed)

👍 that is a clever approach; I like it.

👍 from me on @astrofrog's idea, too.

It should be mentioned in the docs pretty clearly what's going on here, though (in a "note" or other offset section). I would imagine a fair number of novices will be using astroquery, so we want it to be clear what's going on for those that want to try to understand.

Related to the possibility of using these mixed static/instance methods, what do you think about not having the user worry about the sub-packages, i.e. since each sub-package defines a single class, one could just do:

from astroquery import UKIDSS
UKIDSS.query(...)

of course the code can still live in astroquery, but what do you think about the idea of importing the classes to the top-level?

This needs to be finalized before GSoC kicks off, so shall we plan a 1hr telecon to finalize this?

1. 👍 to top-level imports
2. Yes, let's telecon this week. I'll e-mail astropy-dev

There is one tricky bit if you try to use the "class or instance" approach: if you treat it as a classmethod, the execute or query function needs to parse all of the arguments, but the __init__ method already needs to do that in the current API suggestion

I think this is somewhat unclear in the current document: the current API, as suggested, will have a unique class instance for each query. This differs from most of the current implementations, which build the queries as HTTP post objects in the various get_ methods. So far, only SIMBAD is built this way, and I think its structure makes quite a lot of sense once you understand it.

This approach still allows top-level imports, but (using @astrofrog's previous example) UKIDSS would be the module, which would have query_catalog and get_images functions defined.

Merging this so that it gets built on rtfd. Discussion can continue here, but after the telecon we'll have a new PR. Also, I'd like people to be able to implement suggested changes as PRs onto master.