You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@climate.apache.org by Maziyar Boustani <bo...@apache.org> on 2014/01/09 19:12:49 UTC
Review Request 16757: RCMES new command line tool with using latest OCW code
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16757/
-----------------------------------------------------------
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
Re: Review Request 16757: RCMES new command line tool with using latest OCW
code
Posted by Michael Joyce <jo...@apache.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16757/#review40208
-----------------------------------------------------------
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73142>
If you need to convert the string to an number use int() or float(). It's extremely unlikely that you need to use eval. In the very unlikely case that you need to be using eval, then you should be using ast.literal_eval. In this case you should be using float().
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73143>
Don't use eval, see above.
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73144>
The point wasn't to make a function to append to the list. The point was to make a function for formatting the dataset object into something comprehensible. Your choice ultimately.
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73145>
Use built in libraries for path stuff
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73147>
Use os.path for this. Don't make the paths 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/#comment73146>
Is the working directory a path? Are you changing the working directory value? Then you're changing a path =D
It seems the entire point of this if-block is so you can incorrectly generate a file path on line 373. Use os.path for path related things. You don't have to do nit-picky things like adding trailing slashes. Plus, if this ends up running on Windows then you don't have to come back through later and fix all of these explicit path separators that won't work. You'll thank yourself later if you put in the initial effort to do it properly =D
- 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
>
>
Re: Review Request 16757: RCMES new command line tool with using latest OCW
code
Posted by Michael Joyce <ml...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 16757: RCMES new command line tool with using latest OCW
code
Posted by Maziyar Boustani <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16757/#review39796
-----------------------------------------------------------
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72390>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72391>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72856>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72857>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72392>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72393>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72395>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72397>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72396>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72398>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72400>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72401>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72403>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72406>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72408>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72409>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72413>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72492>
done, changed to "model_path"
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72493>
done, changed to "model_path"
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72658>
made two separate try-except block with using else. done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73049>
done,
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72414>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72660>
couple of not necessary line moved out of try block, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72661>
couple of not necessary line moved out of try block, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73048>
done.
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72415>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72416>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72417>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72418>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72419>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72420>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72421>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73047>
if the screen size is small, it cannot add more string and will break the code, so the except block will get it and instead print a better info, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73058>
If I don't put eval here, I think this obs_info['bounding_box'][2][0] will be considered as string and return '8'
but with eval it will return the whole number such as 86.5
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73046>
done,
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73055>
I think if the eval function fails, it can get the except
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73052>
I just append to list, I don't think we need a function for that. What you think ?
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73045>
done,
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72422>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72663>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72664>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73044>
done,
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72423>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72499>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72424>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72916>
docs have been added, no need for more functions here I guess, it is just series of processing and printing outputs
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72917>
docs have been added, no need for more functions here I guess, it is just series of processing and printing outputs
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72425>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72913>
moved down as global variable, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72426>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72427>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72428>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72429>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72430>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72431>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72500>
removed, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72501>
removed, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72432>
removed, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72503>
removed, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72502>
removed, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72504>
removed, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72433>
removed, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72505>
removed, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72434>
removed, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72838>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72435>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72840>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72921>
done,
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72922>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73042>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72923>
done,
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73050>
if the time is given is not in the required format. I added a comment on except block, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73043>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72924>
done,
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72925>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72858>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73054>
done,
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72859>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73053>
done,
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72852>
removed for now, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72854>
removed for now, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72853>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72855>
done,
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment73051>
I am not changing path, I am just changing the working dir value.
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72666>
yes, the note has been removed because I define a note = "" as default in ready_screen function to remove any previous 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/#comment72667>
yes, the note has been removed because I define a note = "" as default in ready_screen function to remove any previous 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/#comment72824>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72825>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72826>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72827>
done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72829>
good idea, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72830>
edited, done
https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/python/rcmes/cli/rcmet_cml.py
<https://reviews.apache.org/r/16757/#comment72833>
modified, done
- Maziyar Boustani
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
>
>