Design/forecast app #1386
Design/forecast app #1386
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
shellcheck found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
HenryRWinterbottom
requested review from
WalterKolczynski-NOAA,
CoryMartin-NOAA,
RussTreadon-NOAA and
guillaumevernieres
There was a problem hiding this comment.
Looks like a good skeleton to start with. Issues are mostly things that aren't your fault.
Made comments on the first j-job I saw, but they apply to all. I know a lot of the things pointed out were from the scripts being emulated, but if we are writing from scratch we should probably not repeat the shortcomings if we can help it (@aerorahul can overrule me here). Also, new shell scripts should come up clean in shellcheck.
| if [ ${DO_WAVE:-"NO"} = "YES" ]; then | ||
| # WAVE component directory | ||
| export CDUMPwave=${CDUMPwave:-${CDUMP}wave} | ||
| export COMINwave=${COMINwave:-$(compath.py ${envir}/${NET}/${gfs_ver})/${CDUMP}.${PDY}/${cyc}/wave} |
There was a problem hiding this comment.
compath.py should not be used to refer to paths within the GFS package (see #289).
| export COMINwave=${COMINwave:-$(compath.py ${envir}/${NET}/${gfs_ver})/${CDUMP}.${PDY}/${cyc}/wave} | |
| export COMINwave=${COMINwave:-${COMROOT}/${NET}/${envir}/${RUN}.${PDY}/${cyc}/wave} |
| # WAVE component directory | ||
| export CDUMPwave=${CDUMPwave:-${CDUMP}wave} | ||
| export COMINwave=${COMINwave:-$(compath.py ${envir}/${NET}/${gfs_ver})/${CDUMP}.${PDY}/${cyc}/wave} | ||
| export COMOUTwave=${COMOUTwave:-$(compath.py -o ${NET}/${gfs_ver})/${CDUMP}.${PDY}/${cyc}/wave} |
There was a problem hiding this comment.
| export COMOUTwave=${COMOUTwave:-$(compath.py -o ${NET}/${gfs_ver})/${CDUMP}.${PDY}/${cyc}/wave} | |
| export COMOUTwave=${COMOUTwave:-${COMROOT}/${NET}/${envir}/${RUN}.${PDY}/${cyc}/wave} |
| rCDUMP=${CDUMP} | ||
| [[ ${CDUMP} = "gfs" ]] && export rCDUMP="gdas" |
There was a problem hiding this comment.
| rCDUMP=${CDUMP} | |
| [[ ${CDUMP} = "gfs" ]] && export rCDUMP="gdas" | |
| export rCDUMP="gdas" |
There was a problem hiding this comment.
@WalterKolczynski-NOAA I just made a copy of file had in another branch. I am guess my rebase didn't take. Before any development/refactoring proceeds I will make sure I begin from a branch UTD with develop. Thank you for the heads-up.
There was a problem hiding this comment.
This is in develop too. Trying not to repeat its mistakes. CDUMP can only be gdas or gfs (enkfgdas and enkfgfs have a different j-job for now). Changing gfs to gdas just means it is always gdas, so let's just say that. (If the enkf script gets combined into this one, I think it would instead be rCDUMP=${RUN/gfs/gdas}.)
|
|
||
| # ---- | ||
|
|
||
| fix_ugwd_: |
There was a problem hiding this comment.
Why a trailing underscore here?
There was a problem hiding this comment.
@WalterKolczynski-NOAA Likely a typo. Thanks for catching it.
|
Mostly looks good. |
|
@HenryWinterbottom-NOAA Did you push your changes to GitHub? I don't see any since my last review. |
|
I think this PR served its purpose for having a discussion. |
The intention of this PR is to provide an easy to review design template, specific to the forecast task refactoring. It does require merging or rebasing and may be removed following completion of the respective task refactoring.
Notes/comments are included in the changed files.
I believe it to be advantageous to drive all task steps initiated from the
scriptslevel (e.g., initialize, run, and finalize) using YAML-formatted files. The YAML-formatted filepath would be defined within the run-time environment. See the notes placed within the YAML-formatted files beneathparm/config/yaml.