----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12072/#review22360 -----------------------------------------------------------
https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45874> These should be removed. svn catches all of this information so we don't have to manually. https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45863> Class names need to be Capitalized. So change this to: class Dataset: https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45865> The repetitive use of 'Values' in the variables names makes me think they are not adding value. My suggestion is using a plural variable name when talking about arrays or lists. Such as: latitudes, longitudes, times, values, name I dropped the valueVariableName because we should instead have a simple name/label since this object is not a NetCDF4 or Nio object, so the variable name isn't really required. The user should have the freedom to change say 'pcp' into 'Precipitation from TRMM version 7.0' as an example. https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45869> So the docstrings get parsed as proper restructured text for Sphinx each heading need to be followed with 2 colons. Purpose:: The initial function is assigning... https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45870> This need to return a tuple per the Class docstring. return (minimumLatitude, maximumLatitude, minimumLongitude, maximumLongitude) Also the variable names should be singular since each one is a single value and not a list. https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45871> Needs to be a tuple https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45872> Should be a tuple https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment45873> You should just sort the times once. This is trivial with a small dataset, but a large one could make a noticeable difference. sortedTimes = numpy.sort(self.times) timeResolution = sortedTimes[1] - sortedTimes[0] We should also wrap the timeResolution with a try block because if a dataset has a SINGLE time value this will throw an indexError when attempting to access sortedTimes[1] - Cameron Goodale On June 25, 2013, 12:35 a.m., Maziyar Boustani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12072/ > ----------------------------------------------------------- > > (Updated June 25, 2013, 12:35 a.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 > >
