jobovy / apogee

Tools for dealing with APOGEE data
BSD 3-Clause "New" or "Revised" License
42 stars 25 forks source link

nearly python3 friendly #31

Closed mrawls closed 8 years ago

mrawls commented 8 years ago

This makes apogee nearly compatible with python3, except for the esutil dependency. esutil has a problem with how some of its underlying C code talks to numpy arrays, so I pretended it isn't required in order to get apogee to install. It is still not usable, however, because import apogee.tools.read as apread lets me get as far as an esutil ImportError. I also updated the ferre URLs.

jobovy commented 8 years ago

Thanks! So does esutil not install during python setup.py install and that's why you removed it from the dependency list?

esutil is essentially only used in apogee.tools.read (the other files where it's used are not really 'core' apogee). But it is quite necessary there (at least esutil.numpy_util.add_fields). I guess a short term solution would be to make esutil optional and raise a warning when things that esutil are necessary for are skipped (like adding J0 etc.). Longer term I guess these lines could be replaced by something that does support python3 (astropy probably has something similar?).

Have you tested the new version of FERRE and does it still work in the same way?

mrawls commented 8 years ago

Right, esutil is the thing that broke the installation for me, and removing that dependency at least allowed me to install apogee. I didn't have time to go through and find all the places where esutil is used, but it was disheartening to see I couldn't even import apogee.tools.read as apread. The issue I commented on over at the esutil repo got a reply essentially saying "making this python3 friendly will be hard and is not happening soon." I like your idea to replace this functionality with something from astropy.

I'm not very familiar with FERRE and I haven't tested it; I was more or less trying every installation option that existed in the apogee README and was happy to be able to easily fix a mere broken link :smile:

For now, I'll put the esutil dependency back in, but add a note to the README that apogee currently only supports python2 so others don't have the same misadventures as me. I'll also add explicit instructions for setting environment variables to the README.

Fair warning, I'm not sure if the relative import syntax I edited or the .decode("utf-8") I added in a few places will still work in python2 or not.

jobovy commented 8 years ago

Okay thanks. I will make it so that apogee.tools.read loads without esutil and check the .decode("utf-8") statements in python 2.7. I'll merge this pull request once I have enough time to do this and test it. Do you need this urgently?

mrawls commented 8 years ago

That sounds great. Not urgent, but I'm looking forward to being able to use this! Thanks.

jobovy commented 8 years ago

I've now merged this and then made the esutil dependency optional. You should be able to load the apogee.tools.read module now in python 3 (please test!). You won't be able to get extinction-corrected magnitudes or to load the APOKASC catalog until there is a work-around for the necessary esutil functions, but otherwise the read functions should work.

See #32.

mrawls commented 8 years ago

Awesome, thank you! It seems to work now! The only bug I found was when I set it to use dr13, RESULTS_VERS=l30e.2, the apStarPath function in apogee.tools.path failed to return anything and I got an error because the path was NoneType. (I switched back to dr12 and it worked fine.) I'll raise new issues if I find further problems. I'm really looking forward to using this code and having some students use it too!

jobovy commented 8 years ago

Many of the paths haven't been updated for DR13 yet (it's only been out for a week!). I updated the apStar and aspcapStar paths and I think the allStar, allVisit, and rcsample paths should also work; but I haven't tested any of these. If you want, please test and if it doesn't work maybe open an issue to track all of the DR13 paths.

jobovy commented 8 years ago

With the new esutil, loading allStar seems to work now in python 3. I had to remove one of the decode that you put in, because it didn't work for me under python 3.4 and the code runs without the decode for me.