delete old replace with new script - #163
Conversation
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR consolidates workflow execution by replacing the standalone forecast script with an updated run_all.sh, and moves user-configurable options to the top of the script for easier customization.
Changes:
- Adds a user-configuration block at the top of
run_all.sh, including toggles to run hindcast and/or forecast. - Extends
run_all.shto create forecast directory structure and execute forecast download/products/plot steps. - Deletes
scripts/master.forecast.shas part of replacing old scripts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/run_all.sh | Centralizes configuration, adds do_hc/do_fc flow control, and integrates forecast execution into the main run script. |
| scripts/master.forecast.sh | Removed in favor of the consolidated run_all.sh workflow. |
Comments suppressed due to low confidence (3)
scripts/run_all.sh:31
base_pathis computed from$SCRATCHbefore any validation, andset -uis enabled later. IfSCRATCHis unset/empty, this silently writes into/osop/...(or another unintended location), which is risky on shared systems. Consider assertingSCRATCHis set and quoting path construction.
scripts/run_all.sh:72- Unquoted path variables in
mkdir -pcan word-split or glob if any directory component contains spaces or wildcard characters, and can also mis-handle paths beginning with-. Quoting and using--makes these directory creations more robust.
scripts/run_all.sh:125 cpshould quote paths (and ideally use an explicit destination filename) to avoid word-splitting/globbing issues and to make it clear the intent is to copy the file into the forecast downloads directory.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
scripts/run_all.sh:86
- PYTHONPATH export will prepend a leading ":" when PYTHONPATH is unset/empty, which implicitly adds the current working directory to sys.path. That’s a security footgun and also regresses the safer pattern previously used in this script.
scripts/run_all.sh:261 - In the forecast path, the script continues into forecast_products/forecast_plots even when the per-centre download fails. This will typically cascade into avoidable failures/noisy logs; the hindcast path already
continues on download failure, so forecast should mirror that behavior.
scripts/run_all.sh:126 - With
do_hc=0, the script still rewrites$downloaddir/parseyml.ymland then copies it into the forecast downloads. That can break “forecast-only” runs that rely on previously-downloaded hindcast data, becauseget_any_hindcast.py/forecast_products.pyuseparseyml.ymlto determine system IDs and will now see the new mapping rather than the one that matches existing hindcast files.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
scripts/run_all.sh:262
- In the forecast branch, a failed download currently falls through to
forecast_products.py/forecast_plots.py. That can waste runtime and can produce misleading downstream errors when inputs are missing. Consider skipping the remainder of the loop for this centre when the download step fails (mirrors the hindcast path above).
scripts/run_all.sh:287 - This error message still references running
master.sh, but this script now combines hindcast+forecast and is intended to replace the older scripts. Updating the message to describe the actual prerequisite (hindcast products/terciles exist for the same settings) will make failures easier to diagnose.
scripts/run_all.sh:16 - This PR deletes
scripts/master.forecast.sh, but there are still user-facing references to that script elsewhere (e.g. docs and error messages). This will mislead users and can complicate troubleshooting. Please update those references to point to the new workflow/script name.
|
also makes the default no borders |
opened #165 to check these |
replace the two old scripts
also move all user configurable options to top of script