Refractors run_analysis to support generalization #26
Conversation
|
@milenaveneziani, do you mind taking a quick look at this and can I still test in the standard way on edison? @xylar obviously should review the code because he wrote |
| return False | ||
| errmsg = "Path %s not found. Exiting..." % inpath | ||
| if warn: | ||
| print 'WARNING!!! ' + errmsg |
There was a problem hiding this comment.
Quick question: when would the Warning be issued in this case?
There was a problem hiding this comment.
warn is a flag that the user can specify. It defaults to False. This would be useful if you just wanted to see if a file or directory exists but don't want to raise a SystemError. We can get rid of this if you don't think it is useful because it is vestigial code. I used it to get around the line if config.get('case', 'ref_casename_v0') is not 'None': below but ended up using that form instead of this function with warn=True because it was cleaner.
There was a problem hiding this comment.
I'll remove it for now. We can always add it back if we need it.
|
|
||
| if config.get('case', 'ref_casename_v0') is not 'None': | ||
| indir_v0data = path_existence(config, 'paths','ref_archive_v0_ocndir') | ||
| indir_v0data = path_existence(config, 'paths','ref_archive_v0_seaicedir') |
There was a problem hiding this comment.
We don't really need indir_v0data at this point, right? We had it before only to check on its existence, but it seems that path_existence(...) should take care of this by itself.
There was a problem hiding this comment.
You are correct. The argument doesn't need to be stored and I'll remove it.
| usesi = config.getboolean('seaice_timeseries', 'generate') | ||
| sicompobs = config.getboolean('seaice_timeseries', 'compare_with_obs') | ||
| simodelvobs = config.getboolean('seaice_modelvsobs', 'generate') | ||
| if (usesi and sicompobs) or simodelvobs: |
There was a problem hiding this comment.
Could you possibly change the new variable names to make them more readable? Something like:
usesi --> use_seaice
sicompobs --> seaice_compare_obs
simodelvobs --> seaice_modelvsobs
There was a problem hiding this comment.
PEP8 standards are that names with underscores are for functions, but surprisingly it doesn't clearly specify variable names. Also, typically variable names should be 5-10 chars long, hence my choice of variable names. I was trying to avoid aliasing with known usage of functions but you are right, it is better in this case and I'll make this change.
| if (usesi and sicompobs) or simodelvobs: | ||
| # we will need sea-ice observations. Make sure they're there | ||
| for obsFile in ['obs_iceareaNH', 'obs_iceareaSH', 'obs_icevolNH', 'obs_icevolSH']: | ||
| obs_filename = path_existence('seaIceData', obsFile, ignorestr='none') |
There was a problem hiding this comment.
Same comment as above about the need for obs_filename.
There was a problem hiding this comment.
Agreed, it isn't needed.
|
@pwolfram: I added some comments in the review. Thanks for submitting this PR. |
pwolfram
force-pushed
the
run_analysis_refractor
branch
from
579830d to
397cd3b
Compare
|
@milenaveneziani, I've made the changes as you requested. The next step is to test this on edison. |
pwolfram
force-pushed
the
run_analysis_refractor
branch
from
397cd3b to
a5783ef
Compare
|
@pwolfram: I will test it in standalone on IC. Running on edison/within the ACME script means dealing with submodule and a non-merged branch, which I am not sure how to deal with right now. |
| # Checks on directory/files existence: | ||
| indir = path_existence(config, 'paths', 'archive_dir_ocn') | ||
|
|
||
| if config.get('case', 'ref_casename_v0') is not 'None': |
There was a problem hiding this comment.
Sorry @pwolfram, I neglected this before: the syntax should go back to:
if config.get('case', 'ref_casename_v0') != 'None':
because anything that is defined in the config file (with or without quotes) becomes a character and this module has no ways to recognize the special variable None. I went through this in PR #25.
There was a problem hiding this comment.
@milenaveneziani, doesn't the config return a string? 'None' is also a string so this shouldn't cause problems.
There was a problem hiding this comment.
I should mention this is confusing because None is a special python character and 'None' is the string "None".
There was a problem hiding this comment.
To test this in PR #25, I had set ref_case_v0 to None (no quotes) in the config file, but the variable was returned as character in the python driver. It was not recognized as the variable None.
There was a problem hiding this comment.
@milenaveneziani, does this mean that the config file recognized it as the special character None? It sounds like this may be the case. This isn't a big deal either way and I reverted it. Thanks for catching this potential issue.
There was a problem hiding this comment.
I only know that whatever I was setting in the config file (quotes or no quotes), the python driver was seeing it as character "None", so the if statement was never satisfied with the syntax 'is not None'. So this was a real issue, and only treating the variable as character everywhere would solve the problem.
There was a problem hiding this comment.
This makes sense now, thanks for clarifying!
|
@milenaveneziani, I tested this on edison in the sub-module / non-merged branch and results are here /global/project/projectdirs/acme/pwolfram/coupled_diagnostics_20160805.A_WCYCL1850.ne30_oEC.edison.alpha7_00-20160520.A_WCYCL1850.ne30_oEC.edison.alpha6_01/index.html and all the ocean/ice plots appear to be working correctly. |
|
@pwolfram: there is still the ref_case_v0 = None issue. I think if you set that, you will obtain similar plots (with ACME v0 stuff added to them) because the v0 data will be read anyway. |
pwolfram
force-pushed
the
run_analysis_refractor
branch
from
a5783ef to
c6a60a8
Compare
|
@milenaveneziani, I fixed that issue above and you should be able to do a reset to get the commit to test under this case. Sorry about the confusion. |
|
@milenaveneziani, as far as I'm concerned this should be ready to commit, assuming the issue with |
|
ok, great. We should keep this (and near-future commits) on the MPAS repo only, though, and commit the changes in the ACME repo after next week. Thanks @pwolfram. |
|
Agreed @milenaveneziani, thanks for the help with this PR! |
Results from this merge should be identical to the previous version of the run script and the refractor is in preparation to help meet needs in issues, e.g., #20.