-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
First pass at subsetter.py #93
Conversation
Warning, treated as error:
Not sure how to resolve. |
@dwfncar I think you have to add it to the top-level Alternatively, if you prepend |
Just noting expandvars (#91) should be merged before this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few suggestions.
scripts/subsetter.py
Outdated
file_out = '/'.join(file_subdirs) | ||
command = 'ncks -O ' + var_str + ' ' + file_in + ' ' + file_out | ||
logging.info(command) | ||
os.system(command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using subprocess.run
is recommended over os.system
. Note that you have to give the command as a list of parts in that case. Can use check=True
so exception is raised if the command gives nonzero exit code.
for var in control['model'][model]['variables']: | ||
var_str += var + ',' | ||
|
||
var_str = var_str.strip(',') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like could use ','.join(...)
instead
Other than addressing my comment above about the deflation level I think this looks good. I agree with the suggestions that @zmoon made. |
Co-authored-by: Zachary Moon <zmoon92@gmail.com>
Co-authored-by: Zachary Moon <zmoon92@gmail.com>
Co-authored-by: Zachary Moon <zmoon92@gmail.com>
Co-authored-by: Zachary Moon <zmoon92@gmail.com>
Co-authored-by: Zachary Moon <zmoon92@gmail.com>
Co-authored-by: Zachary Moon <zmoon92@gmail.com>
Co-authored-by: Zachary Moon <zmoon92@gmail.com>
Co-authored-by: Zachary Moon <zmoon92@gmail.com>
@dwfncar I saw you added the compression level argument and it seems like the other comments have been addressed. Is this finished? If so I'll go ahead and approve it. |
I have one more change to make ... will have ready by this evening. |
@@ -18,9 +18,11 @@ | |||
parser.add_argument('--outdir', type=str, | |||
default=None, | |||
help='output directory') | |||
parser.add_argument('--compress_level', type=int, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parser.add_argument('--compress_level', type=int, | |
parser.add_argument('--compress-level', type=int, |
Personally, I prefer this style, but up to you.
This is ready to merge ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually can you change this first?
Co-authored-by: Zachary Moon <zmoon92@gmail.com>
This script is intended as a stand-alone pre-processor to invoke NCO operators
for subsetting model datasets.
It uses the the same control file as driver.py.
Documentation will be added after early reviews and modifications.