-
Notifications
You must be signed in to change notification settings - Fork 7
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
Ensemble scripts #55
base: develop
Are you sure you want to change the base?
Ensemble scripts #55
Conversation
@wcarthur here's the ensemble scripts - I just created a draft PR for vidsibility |
Codecov Report
@@ Coverage Diff @@
## develop #55 +/- ##
===========================================
- Coverage 86.40% 86.29% -0.11%
===========================================
Files 26 26
Lines 1839 1839
===========================================
- Hits 1589 1587 -2
- Misses 250 252 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ensemble/run_percentiles.sh
Outdated
|
||
export PYTHONPATH=$PYTHONPATH:$SOFTWARE/tcrm/$BRANCH:$SOFTWARE/tcrm/$BRANCH/Utilities | ||
|
||
cd /g/data/w85/kr4383/yasi |
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.
Can you confirm permissions have been changed to 775 on these folders
ensemble/loss_percentiles.py
Outdated
|
||
# only these filepaths need to be specified | ||
low_res_impact_dir = os.path.join("impact", "low_res") | ||
working_dir = "/g/data/w85/kr4383/yasi/percentile" |
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.
Should be a configuration option or a command line argument to the script. I usually have an
if __name__=='__main__':
clause, followed by using argparse
to get command line arguments for these types of variables. Means we can commit the code, run different versions and show (using the commit hash) that we're using the same code each time. If we have to change the paths in the file, might show up as multiple changes, or we end up with code like
# low_res_impact_dir = /foo/bar
low_res_impact_dir = /foo/baz
ensemble/run_percentiles.sh
Outdated
export PYTHONPATH=$PYTHONPATH:$SOFTWARE/tcrm/$BRANCH:$SOFTWARE/tcrm/$BRANCH/Utilities | ||
|
||
cd /g/data/w85/kr4383/yasi | ||
python3 loss_percentiles.py |
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.
Is there any STDOUT messages that could be logged to help troubleshoot problems
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.
yep the STDOUT from hazimp and wind multipliers is captured and sent to files
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.
Some comments on each file to be addressed. I prefer not to hard-code paths to input folders, rather they should be command line arguments that can be specified in the shell script that calls the python code.
See https://github.com/wcarthur/processing/blob/master/process/access/process_windgust.py and the start()
function for a really comprehensive implementation. In that case, running the script defines all the functions, gets to the bottom and calls start()
which handles all the command-line arguments using teh argparse
module, sets up log files, then calls the functions that actually do the work. This one doesn't demand quite such a comprehensive implementation, but command line args for the paths would be helpful
Make sure that the shell script sets umask 002
so the rest of the project members can read and write to the folders used (may need to check permissions if you've not got time to run the scripts).
@wcarthur I think I addressed your comments if you time to look over it again |
No description provided.