climate-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Joyce" <jo...@apache.com>
Subject Re: Review Request 16757: RCMES new command line tool with using latest OCW code
Date Sat, 12 Apr 2014 16:50:21 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message