----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12072/#review22455 -----------------------------------------------------------
https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment46007> List them explicitly in the docstring for the class is what I would do. https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment46003> 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. https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment46004> If we're sorting the times why do we need to check if timeResolution.days > 1? We can't end up with a negative time delta because of the sort and we already checked if timeResolution.days == 0 https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment46005> Perhaps my logic is wrong here so maybe someone can set me straight. If we have a bunch of time values shouldn't the time delta for yearly data be timeResolution.days > 365? Or do we consider anything that isn't monthly to be yearly? If that's the case wouldn't the previous elif be wrong as well? https://svn.apache.org/repos/asf/incubator/climate/trunk/dataset.py <https://reviews.apache.org/r/12072/#comment46006> Don't use prints. Use a logger or throw an exception. Never ever use a print. - 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 > >
