mangonetwork / mangopy

This repository contains code for processing all-sky images generated by the Mid-latitude All-sky-imager Network for Geophysical Observations (MANGO), a project funded by the National Science Foundation.
GNU General Public License v3.0
6 stars 2 forks source link

Review for first release of mangopy #3

Closed ljlamarche closed 5 years ago

ljlamarche commented 5 years ago

This is for the first release of mangopy. Please take a look and provide suggestions for any/all aspects of it (code, documentation, package installation, style, ect.)!

asreimer commented 5 years ago

Looks good to me. In addition to the comments in my review, I have a couple more: 1) What versions of python is this code working in? 2) The README could include some general information about how the package works, specifically: mangopy puts mango files in a data directory and if they don't exists there, they are pulled via FTP. 3) The tutorial is great! But the last line in it has a problem, see: https://github.com/astib/MANGO/blob/develop/mangopy_tutorial.ipynb

ljlamarche commented 5 years ago

Thanks @asreimer ! To address your questions:

  1. I've been testing this in python 3.6, but I'll give it a try in python 2.7.
  2. Good point about the README! I'll add that in.
  3. The last line in the tutorial is actually expected (although probably undesirable) behavior. The root of the issue is that there are often time when not all sites have data available. Currently, the mosaic code spits out a lot of errors when this happens and skips to the next site, but maybe it's ok to hide some of those exceptions, at least by default. Maybe we can put in a verbose option?
asreimer commented 5 years ago

EDIT: this is a more useful link: https://github.com/pypa/sampleproject

Hmmm. It seems that one of my comments about the setup.py didn't make it into the original review. One can use the install_requires keyword to specify the dependencies. It is suggested that one does this using a requirements.txt file: https://packaging.python.org/discussions/install-requires-vs-requirements/

or even better, a setup.cfg file: https://docs.python.org/3/distutils/configfile.html

asreimer commented 5 years ago

Everything works great on python 3.7.3.

asreimer commented 5 years ago

Works perfectly in Python 2.7, but only if the matplotlib requirement is changed from >=3.0 to >=2.2.4.

Edit: only works if you run python 3 first because it has already downloaded the files then. urllib problem in Python 2.7:

from mangopy import Mango
man = Mango()
site = man.get_site_info('Capitol Reef Field Station')
import datetime as dt
time0 = dt.datetime(2016,4,10,5,30)
man.plot(site,time0)

---------------------------------------------------------------------------
IOError                                   Traceback (most recent call last)
<ipython-input-4-a8767e88bff4> in <module>()
      1 import datetime as dt
      2 time0 = dt.datetime(2016,4,10,5,30)
----> 3 man.plot(site,time0)

/home/asreimer/virtualenvs/usrvenv/lib/python2.7/site-packages/mangopy/mango.pyc in plot(self, site, targtime)
     34     def plot(self,site,targtime):
     35         # plot single mango image
---> 36         img, __, __, truetime = self.get_data(site,targtime)
     37         plt.imshow(img, cmap=plt.get_cmap('gist_heat'))
     38         plt.title('{:%Y-%m-%d %H:%M}'.format(truetime))

/home/asreimer/virtualenvs/usrvenv/lib/python2.7/site-packages/mangopy/mango.pyc in get_data(self, site, targtime)
     66         # first try to read data file locally
     67         try:
---> 68             img_array, lat, lon, truetime = self.read_datafile(filename,targtime)
     69         # if that fails, try to download, then read the data file
     70         except OSError:

/home/asreimer/virtualenvs/usrvenv/lib/python2.7/site-packages/mangopy/mango.pyc in read_datafile(self, filename, targtime)
     77 
     78     def read_datafile(self,filename,targtime):
---> 79         with h5py.File(filename, 'r') as file:
     80             tstmp0 = (targtime-dt.datetime.utcfromtimestamp(0)).total_seconds()
     81             tstmp = file['Time'][:]

/home/asreimer/virtualenvs/usrvenv/lib/python2.7/site-packages/h5py/_hl/files.pyc in __init__(self, name, mode, driver, libver, userblock_size, swmr, rdcc_nslots, rdcc_nbytes, rdcc_w0,     track_order, **kwds)
    392                 fid = make_fid(name, mode, userblock_size,
    393                                fapl, fcpl=make_fcpl(track_order=track_order),
--> 394                                swmr=swmr)
    395 
    396             if swmr_support:

/home/asreimer/virtualenvs/usrvenv/lib/python2.7/site-packages/h5py/_hl/files.pyc in make_fid(name, mode, userblock_size, fapl, fcpl, swmr)
    168         if swmr and swmr_support:
    169             flags |= h5f.ACC_SWMR_READ
--> 170         fid = h5f.open(name, flags, fapl=fapl)
    171     elif mode == 'r+':
    172         fid = h5f.open(name, h5f.ACC_RDWR, fapl=fapl)

h5py/_objects.pyx in h5py._objects.with_phil.wrapper()

h5py/_objects.pyx in h5py._objects.with_phil.wrapper()

h5py/h5f.pyx in h5py.h5f.open()

IOError: Unable to open file (unable to open file: name = '/tmp/MANGOData/Capitol Reef Field Station/Apr1016/CApr1016.h5', errno = 2, error message = 'No such file or directory', flags =     0, o_flags = 0)
ljlamarche commented 5 years ago

Ok, I modified fetch_datafile to use ftplib instead of urllib (requests doesn't actually appear to work for ftp). I also changed requirements.txt as recommended. Based on my testing in conda python 2 and python 3 environments, all of this works now!

Unless there are any more small outstanding issues, I'm fine merging this PR at this point.

asreimer commented 5 years ago

Looks good! All my testing shows things working really nicely in python2 and 3.