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

Preliminary code for Simbad #120

Merged
merged 33 commits into from
Jul 5, 2013
Merged

Preliminary code for Simbad #120

merged 33 commits into from
Jul 5, 2013

Conversation

jdnc
Copy link
Contributor

@jdnc jdnc commented Jun 24, 2013

This is a very very basic shot at re-writing the Simbad module. Most of the implementation is yet to be tried out ... but it would be great to get the initial feedback

]

# add this to a top level common utils?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's a good idea. Separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I'll open a PR for this

@jdnc
Copy link
Contributor Author

jdnc commented Jun 27, 2013

The last few commits add docstrings and some more implementation. I am working on displaying the votable fields. The unicode strings are indeed accepted by astropy.table.Table - I had some confusion on this point initially, but by doing string.encode('utf-8') and specifying the dtype as object, it works now.
EDIT : what I am working on is the display part - currently the second column that describes the field in the first column can only be seen as ellipsis
screenshot from 2013-06-27 18 50 10

@jdnc
Copy link
Contributor Author

jdnc commented Jun 27, 2013

Also need to restructure this to a core.py, removing unused code and add tests...

@jdnc
Copy link
Contributor Author

jdnc commented Jun 30, 2013

Well this restructures the existing code for Simbad - I have removed the code that was no longer used, also the SimbadResult class that was originally in a result.py has been moved to core with some minor changes. The Travis builds are failing as OrderedDict isn't there in python 2.6. This code doesn't actually make any significant use of OrderedDict so it could be easily replaced or we could also add ordereddict to astroquery?

Also most of the implemented functionalities are covered under tests now. One interesting thing I found is that simply monkeypatching requests.post in the tests removes the need to monkeypatch any of the _async routines totally. This can perhaps be a more complete way to test - since all the functions are called and used per se and we only replicate the library function. So I think I'll also do something like this for #110 .

I haven't been able to make much head away with the votable fields display. Another thing is to get the validate decorators to work - again I need to look at this.

@keflavich
Copy link
Contributor

If ordereddict isn't used, we should get rid of it. Otherwise, we should have ordereddict as a requirement for python2.6 users and use collections.OrderedDict for >2.6.

Let's discuss the votable fields display & validate decorators issues on Tuesday.

from . import SIMBAD_SERVER, SIMBAD_TIMEOUT, ROW_LIMIT
__all__ = ['Simbad']

# need to fix, before they work
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you point out the failure / include an ipynb with an example?

@keflavich
Copy link
Contributor

My inline comments are mostly minor.

One thing I'd really like to see with the SIMBAD updates, though, is an improved documentation page. i.e., copy a test of each type, and include it in docs/simbad.rst with a little bit of explanation of what the user input needs to be and what the expected output should look like.

@jdnc
Copy link
Contributor Author

jdnc commented Jul 1, 2013

The validate decorators are working now. I was actually chaining them in the wrong order. What I initially did was

    @validate_epoch
    @validate_equinox
    @class_or_instance
    def _args_to_payloads(self, *args, **kwargs):

This raises an error about getting the wrong argument types.
Now when I tried this:

    @class_or_instance
    @validate_epoch
    @validate_equinox
    def _args_to_payloads(self, *args, **kwargs):

the decorators are working.

@jdnc
Copy link
Contributor Author

jdnc commented Jul 2, 2013

Yes, it will be great to discuss the table display issues at today's telecon. One more thing I was wondering is whether we can also make our HTTP POST requests asynchronous in the true sense by using a framework like
grequests or similar.

@jdnc
Copy link
Contributor Author

jdnc commented Jul 4, 2013

The last few commits add the documentation as well as the code for votable_fields.

@keflavich
Copy link
Contributor

@jdnc I'll look into this

@keflavich
Copy link
Contributor

@jdnc it worked fine for me first try!

In [1]: from astroquery.simbad import Simbad

In [2]: Simbad.list_votable_fields()
--NOTES--

1. The parameter filtername must correspond to an existing filter. Currently: B,V. Coming soon: IR filters (R,I,J,K,L)

2. Fields beginning with rvz display the data as it is in the database. Fields beginning with rv force the display as a radial velocity. Fields beginning with z force the display as a redshift

3. For each measurement catalog, the VOTable contains all fields of the first measurement. When applicable, the first measurement is the mean one.

          col0                   col1              col2
------------------------ -------------------- --------------
      bibcodelist(y1-y2) fluxdata(filtername)       plx_qual
                     cel                 gcrv             pm
                    cl.g                  gen     pm_bibcode
                coo(opt)                   gj   pm_err_angle
             coo_bibcode                 hbet    pm_err_maja
           coo_err_angle                hbet1    pm_err_mina
            coo_err_maja                 hgam        pm_qual
            coo_err_mina              id(opt)          pmdec
                coo_qual                 iras     pmdec_prec
          coo_wavelength                  irc           pmra
             coordinates                  iso      pmra_prec
                dec(opt)                  iue            pos
                dec_prec                 jp11           posa
                     dim              main_id  propermotions
               dim_angle         measurements        ra(opt)
             dim_bibcode               mesplx        ra_prec
                dim_incl                mespm            rot
             dim_majaxis                   mk       rv_value
             dim_minaxis            morphtype           rvel
                dim_qual                   mt    rvz_bibcode
          dim_wavelength           mt_bibcode      rvz_error
              dimensions              mt_qual       rvz_qual
                einstein                  orv     rvz_radvel
                    fe_h                otype       rvz_type
        flux(filtername)           otype(opt) rvz_wavelength
flux_bibcode(filtername)             parallax            sao
  flux_error(filtername)                  plx             sp
   flux_name(filtername)          plx_bibcode     sp_bibcode
flux_quality(filtername)            plx_error      sp_nature
   flux_unit(filtername)             plx_prec        sp_qual
                  sptype                   --             --

For more information on a field :
Simbad.get_field_description ('field_name')

@jdnc
Copy link
Contributor Author

jdnc commented Jul 4, 2013

It works now - moved setup_package.py to simbad/ from simbad/tests/ and added the paths for the json files.

@keflavich
Copy link
Contributor

Think it's ready to merge?

@astrofrog
Copy link
Member

This looks great!

@jdnc
Copy link
Contributor Author

jdnc commented Jul 5, 2013

No more commits pending to be pushed now.

keflavich added a commit that referenced this pull request Jul 5, 2013
Preliminary code for Simbad
@keflavich keflavich merged commit cc74312 into astropy:master Jul 5, 2013
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