----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16757/#review31617 -----------------------------------------------------------
Mazi, Please run your code through Pylint first before requesting a review. That will help a lot of issues get caught and save some time on the reviewer's end. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60315> Shouldn't have JPL Specific branding. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60316> Need to have an ASF compliant header https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60220> I would leave this to netCDF4.Dataset to avoid any confusion with OCW.dataset.Dataset. When I saw this further in the code that's what I assumed was being referenced. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60221> Duplicate import https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60222> You don't need this to be "import Bounds as Bounds". Just "import Bounds" should be fine. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60223> Why import these in addition to importing rcmed above? Just referencing them as rcmed.blah should be sufficient right? https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60215> Reversing the words in the name would make the intention of this function clearer. "ready_screen" instead of "screen_ready". "Screen_ready" sounds like a test. For instance, "if screen_ready: do something" whereas "ready_screen" sounds more appropriate for a function call. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60211> Need more descriptive function docstrings. What are the inputs/outputs? Etc. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60212> There a lot of whitespace issues. I.e., "y,x" should be "y, x". Running the code through PyLint should give you a good starting place for changes that need to be made. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60213> Where are we getting "x/2-25" from? What is 25? It's half the length of the String being added; Is that relevant? A comment or two would clear up any confusion here (and elsewhere). Hardcoded magic values aren't a good thing. At the very least some explanation of where they're coming from helps. Also, this shouldn't be NASA JPL branded if this is at the ASF. Removing the branding and placing it into a const that can be easily edited is better. A global such as TITLE = "Open Climate Workbench Evaluation System" ORGANIZATION = "Apache Software Foundation" This makes modifications easier if it is rebranded. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60214> Is there a reason this is being returned if the values can be easily grabbed with a call to "screen.getmaxyx()"? Especially since only one of the calls to the function actually use the return value. Similarly, "screen_ready" doesn't imply to me that a return value should be expected. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60217> See previous docstring comment. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60216> screen_ready is called most often with an empty string for the note. It makes more sense to make note default to an empty string so you only need to pass the page name. There's also no point in using a positional argument for the note value while using a keyword argument for the page value since there aren't any other possibilities. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60224> Is this returning a directory or a path? It doesn't make much sense for this to be a directory so I imagine this is just a bad name. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60226> These try..except blocks need some love and attention. Nesting try..except blocks is less than ideal. At the moment it is challenging to see what exactly could raise an exception versus what is doing other work. try: netCDF_File = netcdf4.Dataset(model_directory, 'r') except TheActualErrorNameHere: return "WARNING: Model file cannot be..." all_netcdf_variables = ... netCDF_file.close() screen.addstr(... variable_name = screen.getstr() try: model_dataset = load_file(... except ValueError: return "WARNING: ..." model_datasets.append(... models_info.append(... return "Model file successfully added." https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60227> Need to put the exception name here. More explicit is always better! https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60230> See above docstring comment https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60228> See above try..except comments. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60229> See above specific Except comments. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60231> See above docstring comment https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60232> This is over indented I think. Looks like there's two tabs instead of 1. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60233> I'm not really a fan of returning an empty string here for the purpose of clearing a note. This logic is better off stuck in the calling code in my opinion. if option == '1': note = load_model_screen(header) if option == '2': note = unload_model_screen(header) if option == '3': list_model_screen(header) note = '' https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60234> See above docstring comments https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60235> See above note in list_model_screen regarding setting 'note' https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60236> See above docstring comments https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60239> What exactly in this block is raising an exception? There's a ton of stuff here and it's not obvious what could be raising an exception. If it's only to determine if the screen size is insufficient I think there must be a better way of checking that before trying to write a bunch of stuff out. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60237> Why are we using 'eval' here? 'Eval' is a big no-no. Comments here as to why the indexing is happening would be nice for future debugging/clarity. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60238> Specific Exception name needed https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60240> Again, what is actually raising an exception? https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60242> A function for formatting the object might be nice here. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60241> This else block would do better attached to the for loop instead of this if. At the moment this is repeatedly setting the note for no reason. It also isn't really the most appropriate use of an if..else since there isn't really two branches of logic here. Either: note = "WARNING: ..." for obs in all_obs_info: if ...: ... break or: for obs in all_obs_info: if ...: ... break else: note = "WARNING: .." with a heavy preference for the latter since this is effectively the purpose of the construct. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60244> See above docstring comments. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60246> Less stuff in the try block is better! Makes it clearer what is actually raising the exception. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60247> Specific exception needed https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60250> See above docstring comments https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60249> See the above comment regarding handling resetting the note in this way. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60251> See above docstring comments https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60254> This function needs some comments to clarify various steps. Breaking this up into smaller functions might not be a terrible idea either. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60252> Dosctring again https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60253> If we're defining a constant here we should really be declaring this outside of the function in my opinion. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60255> Docstring again https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60256> Docstring again https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60257> Docstring again https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60258> Docstring again https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60259> Docstring again https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60260> Docstring again https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60262> Do we really need a function for this? Seems like model_datasets.temporal_resolution can be called inline without any side effects. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60261> Docstring again https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60263> Don't really need this function either. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60268> Docstring again https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60265> Don't really need this function. This can be replaced inline with model_lat_res, model_lon_res = model_datasets.spatial_resolution() https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60264> Docstring again https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60266> Don't need this function either. There is only one observation as well so the plural observations is confusing. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60267> Docstring again https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60274> This function needs cleaned up a bit. There's way too much going on in here in my opinion. Breaking up the options into separate functions would make the code far more manageable. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60275> Docstring again https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60283> Separate functions for each of these options would clean this up a lot. The error functions need to be more explicit as well. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60281> More explicit message would be nice here https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60277> Specific exception names https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60280> It seems that we're tying to parse a datetime with a specific format and catching an exception if that doesn't work. However, the error is not very informative to the user. If they want to change the time but enter the format incorrectly we should tell them with a more explicit error message. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60278> What exception are we trying to catch here as well? https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60279> Specific exception name https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60285> See above for this option as well. Also, why aren't we using elif for any block outside of option 1? https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60286> See above for this option as well https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60288> new_temp_grid_option = screen.getstr().lower() removes the two calls to lower in the if..elif..else block https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60289> See above for this option as well https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60292> .lower() here removes the 3 extra calls later on https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60291> Comment this out if you don't want in here. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60293> See above for this option as well https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60295> Use the built in libraries to handle paths and path changes instead of doing it manually. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60302> Does main menu need to be passed a note? Where do we use this? https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60309> Use elif here https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60310> and use elif here https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60311> and use elif here as well https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60313> I would prefer to see checks against the lengths of the lists instead of string comparisons here personally. you can completely remove the model/obs_status variables as well then. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60307> Why are we recursively calling main_menu here? Just set 'note' and let the while loop run. https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py <https://reviews.apache.org/r/16757/#comment60300> Why define globals and then pass them to the function? Not having the globals would be nice, but at the moment you just have the worst of both worlds. None of the other functions that use the globals expect them to be passed, so stuff is mostly just being passed here for no reason it seems? Especially since the other code will fail without these globals being defined. - Michael Joyce On Jan. 9, 2014, 6:12 p.m., Maziyar Boustani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16757/ > ----------------------------------------------------------- > > (Updated Jan. 9, 2014, 6:12 p.m.) > > > Review request for Apache Open Climate. > > > Repository: climate > > > Description > ------- > > This command line tool is using latest version of OCW (Open Climate > Workbench) code to give the capability of evaluating climate model output and > observation with using available metrics along with generating plots as > result. > At this time, this tool can accept one model and one observation and to > generate contour plots with using BIAS as metric. Supporting multi model and > multi observation, more metrics and plots are in coming updates. > > > Diffs > ----- > > > https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py > PRE-CREATION > > Diff: https://reviews.apache.org/r/16757/diff/ > > > Testing > ------- > > This tool has been tested with one model [1] and one observation [2]. > Plots were generated successfully. > > [1]: AFRICA_KNMI-RACMO2.2b_CTL_ERAINT_MM_50km_1989-2008_tasmax.nc > [2]: Dataset_id = 10 and Parameter_id = 39 > > > Thanks, > > Maziyar Boustani > >
