----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12072/#review22405 -----------------------------------------------------------
https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45924> Don't need this new line https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45925> Too many new lines here https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45950> Do we need this timedelta import? I'm not seeing where it is used. Actually, we don't need either of these unless I'm overlooking something. https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45928> Don't say that it "will" do something, say that it "does" something. """Store all relevant attributes for a given dataset. Longer explanation here. """ Remember, the first line has to fit on one PEP8 standard line (which is 80 char I believe). Docs should be terse, present tense, and be written as commands/instructions not a description ('return x' as opposed to 'returns x') More info available at: http://www.python.org/dev/peps/pep-0008/#documentation-strings http://www.python.org/dev/peps/pep-0257/ https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45943> In general, I feel like "latitudes" and "longitudes" is too explicit. "lons" and "lats" are sufficient. I love love love long and descriptive variable names but I think we can make an exception when we're dealing with lat/lon stuff" What does everyone else think about this? https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45931> This needs to be one line and it's a bit overkill for an init function. """Default Dataset init""" is probably sufficient for THIS line. https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45932> "This is the" is just taking up space here. "1-D numpy array of unique latitude values" works fine https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45948> Should probably have a newline in between args and returns. Imagine that would be clearer. In general more newlines would be better https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45933> __init__ isn't returning anything so this stuff shouldn't be here. https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45934> Same thing here regarding tense and fluff. "Return spatial boundaries" https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45923> Need a new line here https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45921> Variable names are plural here and singular in the return on line 80 https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45922> I realize that "decimal number" is intended to mean "float", but there's no data type called "decimal number". When I read that I think you're trying to say a number in base 10. I would recommend changing it to "float" or "double" https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45935> Same thing as above https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45936> Need a new line https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45937> "python datetime" should be "datetime". If you want to be really pedantic you could put "datetime.datetime" since the datetime class is in the datetime module. I think I would just stick with "datetime" We don't need to say we're returning "python" objects. That's pretty much the expected functionality. Similarly, we don't need to specify the return type (datetime) and then say that we're returning it in that format in the description ("in python datetime format"). https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45949> Why are we sorting twice here? Sort self.times once and then index into the results. https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45939> If you need to index into the end of a list in python you can use a negative index. It's much cleaner and more "pythonic" x = [1, 2, 3] x[len(x) - 1] == 3 x[-1] == 3 https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45940> Same thing as above https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45941> Should have a new line here https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45942> Don't need to call list(set()) twice here. Also you can't guarantee the order of elements in a set which means that you might end up with unexpected functionality here. You should really do sortedLats = numpy.sort(list(set(self.lats))) if you wanted unique and sorted lat values https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45944> Same as above https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45945> Need a newline here https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45946> Do the timeResolution.seconds/3600 calculation outside of the if/else block. No point in doing it multiple times if we dont' need to. https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45947> Clean out all these newlines here - Michael Joyce On June 26, 2013, 3:56 p.m., Maziyar Boustani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12072/ > ----------------------------------------------------------- > > (Updated June 26, 2013, 3:56 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 > >
