> On June 27, 2013, 12:57 a.m., Michael Joyce wrote: > > https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py, line 41 > > <https://reviews.apache.org/r/12072/diff/2/?file=311159#file311159line41> > > > > List them explicitly in the docstring for the class is what I would do.
Hey guys. I think i might have the tie breaker here. I think Sphinx will automatically document all class attributes for you. This is much different than a function where you cannot easily tell which variables are doing stuff. Another thing to try is to import this class then call dir() and help() on it within the Python Interpreter. > On June 27, 2013, 12:57 a.m., Michael Joyce wrote: > > https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py, line > > 128 > > <https://reviews.apache.org/r/12072/diff/2/?file=311159#file311159line128> > > > > How is this being used once per model? The calculation is done at most > > twice per call and at least once. Aside from the fact that it's less > > efficient to do this calculation (possibly) twice each time we call this > > function, it makes code clearer if you break it out. > > > > numHours = timeResolution.seconds / 3600 > > if timeResolution.days == 0 and numHours >= 1 > > > > That is more explicit. > > > > That being said it would be better to change the if/else block to > > > > if timeResolution.days == 0: > > numHours = timeResolution.seconds / 3600 > > timeResolution = 'hourly' if numHours >= 1 else 'minutely' > > > > since it breaks the if/else block up more logically with only a single > > check for timeResolution.days == 0. This also means that we don't have to > > do the calculation for number of hours at all if resolution isn't > > hourly/minutely. So we do the calculation 0 or 1 times instead of 1 or 2 > > times. > > > > If we can 1) reduce our calculations done by 50% per call (worst case) > > or completely remove the calculation (best cast) and 2) make the intention > > of the code clearer why would we leave it like it is? > > > > Aside from all that, you should have whitespace around the division > > operator. I agree with Mike. Using numHours is more explicit. Another point: We don't need minutely for a temporal resolution. Talking to the science team the lowest temporal resolution currently is hourly. That said; if numHours < 1: # raise a temporalResolution exception <== Something like that - Cameron ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12072/#review22455 ----------------------------------------------------------- On June 27, 2013, 4:25 p.m., Maziyar Boustani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12072/ > ----------------------------------------------------------- > > (Updated June 27, 2013, 4:25 p.m.) > > > Review request for Apache Open Climate, Cameron Goodale, Chris Mattmann, > Michael Joyce, and Paul Ramirez. > > > Repository: climate > > > Description > ------- > > Based on the new structure of OCW (Open Climate Workbench) [1], the > dataset.py is the critical file that will contain all the functions and > attributes related to any type of dataset (local observations, model file, > RCMED and esg). > This is the first time the dataset.py is developed, therefore it may need to > be modified more as we go ahead. > > [1]: > https://cwiki.apache.org/confluence/display/CLIMATE/Open+Climate+Workbench+API+summary > [STRUCTURE A] > > > Diffs > ----- > > https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py > PRE-CREATION > > Diff: https://reviews.apache.org/r/12072/diff/ > > > Testing > ------- > > There are three files attached to this review board that can provide the test > situation for you. > 1- local.py ---> This file is not the final version of local.py. However, it > is a good source to test the dataset.py but a model file[3]. > 2- test.py ---> This file will use local.py[1] and model file[3] to test all > the attributes from dataset.py. > 3- prec.CRCM.ncep.monavg.nc ---> This is the actual model file that can be > used to test the dataset.py (feel free to use your model file and report if > any issue found) > > > File Attachments > ---------------- > > This is not the final version of local.py. However, the dataset.py can be > test by this local.py (code example in "Testing area"). > https://reviews.apache.org/media/uploaded/files/2013/06/25/local.py > This > https://reviews.apache.org/media/uploaded/files/2013/06/25/test.py > dfd > > https://reviews.apache.org/media/uploaded/files/2013/06/25/prec.CRCM.ncep.monavg.nc > > > Thanks, > > Maziyar Boustani > >
