Add the repair job for the reforecast #2838
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![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 20 potential problems in the proposed changes. Check the Files changed tab for more details.
scripts/gefs_atmos_f0306.sh
Outdated
Comment on lines
151
to
152
| $exec_dir/$sorc_name >sorc_name.exe.out | ||
| cat sorc_name.exe.out |
There was a problem hiding this comment.
@WalterKolczynski-NOAA, @HongGuan-NOAA has source code for this task that has not been added to this PR. Should her source code be added to gfs-utils in a similar manner as the other GEFS-specific programs (e.g. gfs-utils PR #52)?
There was a problem hiding this comment.
Yes. I thought I had already talked with Hong about opening a PR.
scripts/gefs_atmos_f0306.sh
Outdated
| @@ -0,0 +1,192 @@ | |||
| #!/bin/ksh | |||
There was a problem hiding this comment.
This script should start like this. A brief description of this script should be added as well.
Suggested change
| #!/bin/ksh | |
| #! /usr/bin/env bash | |
| source "${USHgfs}/preamble.sh" |
jobs/JGEFS_ATMOS_ACC
Outdated
| # | ||
| #export COMIN_master=${COMIN_master:-$COMROOT/$PSLOT/gefs.$PDY/00/mem001/model_data/atmos/master} | ||
| export COMIN_master=${COMIN_master:-${COM_ATMOS_MASTER}} | ||
| export COMIN_00and03=${HOMEgefs}/anl |
There was a problem hiding this comment.
COMIN_00and03 should not be in HOMEgefs. Since data in this directory is considered fixed (not generated in any global-workflow task), I think it can be somewhere in a common data directory outside of HOMEgefs.
| history_path = self._template_to_rocoto_cycstring(self._base[history_path_tmpl], {'MEMDIR': 'mem#member#'}) | ||
|
|
||
| data = f'{history_path}/{history_file_tmpl}' | ||
| dep_dict = {'type': 'data', 'data': data} |
There was a problem hiding this comment.
Since the repair job depends on f006 master data from the forecast job, I think the dependencies for the f003 and f006 atmos_prod jobs should also be modified (for replay cases). The f003 and f006 atmos_prod jobs should depend on the successful completion of the repair job.
workflow/applications/gefs.py
Outdated
| @@ -59,6 +59,8 @@ def get_task_names(self): | |||
| if self.nens > 0: | |||
| tasks += ['efcs'] | |||
|
|
|||
| tasks += ['repairf0306'] | |||
There was a problem hiding this comment.
I think this task should be added to the workflow only for replay cases. This task should probably be conditioned. I suggest reviewing the extractvars task to see how a task can be made optional in the workflow.
|
I will take a look. Thanks for looking into this. -Hong
…On Mon, Aug 26, 2024 at 8:29 AM Eric Sinsky - NOAA ***@***.***> wrote:
***@***.**** commented on this pull request.
@HongGuan-NOAA <https://github.com/HongGuan-NOAA> Here are some of my
suggestions.
------------------------------
In scripts/gefs_atmos_f0306.sh
<#2838 (comment)>
:
> @@ -0,0 +1,192 @@
+#!/bin/ksh
This script should start like this. A brief description of this script
should be added as well.
⬇️ Suggested change
-#!/bin/ksh
+#! /usr/bin/env bash
+
+source "${USHgfs}/preamble.sh"
------------------------------
In jobs/JGEFS_ATMOS_ACC
<#2838 (comment)>
:
> +
+export PARMgefs=${HOMEgefs}/rocoto/parm
+echo "${PARMgefs}"
+
+# Construct COM variables from templates
+#YMD=${PDY} HH=${cyc} declare_from_tmpl -rx COM_ATMOS_ANALYSIS COM_ATMOS_HISTORY COM_ATMOS_MASTER
+ YMD=${PDY} HH=${cyc} declare_from_tmpl -rx COM_ATMOS_MASTER
+
+
+##############################
+# Run setpdy and initialize PDY variables
+##############################
+#
+#export COMIN_master=${COMIN_master:-$COMROOT/$PSLOT/gefs.$PDY/00/mem001/model_data/atmos/master}
+export COMIN_master=${COMIN_master:-${COM_ATMOS_MASTER}}
+export COMIN_00and03=${HOMEgefs}/anl
COMIN_00and03 should not be in HOMEgefs. Since data in this directory is
considered fixed (not generated in any global-workflow task), I think it
can be somewhere in a common data directory outside of HOMEgefs.
------------------------------
In workflow/rocoto/gefs_tasks.py
<#2838 (comment)>
:
> +
+ def repairf0306(self):
+
+ deps = []
+
+ products_dict = {'atmos': {'config': 'atmos_products',
+ 'history_path_tmpl': 'COM_ATMOS_MASTER_TMPL',
+ 'history_file_tmpl': ***@***.***'}}
+ component_dict = products_dict['atmos']
+ config = component_dict['config']
+ history_path_tmpl = component_dict['history_path_tmpl']
+ history_file_tmpl = component_dict['history_file_tmpl']
+ history_path = self._template_to_rocoto_cycstring(self._base[history_path_tmpl], {'MEMDIR': 'mem#member#'})
+
+ data = f'{history_path}/{history_file_tmpl}'
+ dep_dict = {'type': 'data', 'data': data}
Since the repair job depends on f006 master data from the forecast job, I
think the dependencies for the f003 and f006 atmos_prod jobs should also be
modified (for replay cases). The f003 and f006 atmos_prod jobs should
depend on the successful completion of the repair job.
------------------------------
In workflow/applications/gefs.py
<#2838 (comment)>
:
> @@ -59,6 +59,8 @@ def get_task_names(self):
if self.nens > 0:
tasks += ['efcs']
+ tasks += ['repairf0306']
I think this task should be added to the workflow only for replay cases.
This task should probably be conditioned. I suggest reviewing the
extractvars task to see how a task can be made optional in the workflow.
—
Reply to this email directly, view it on GitHub
<#2838 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVLBRUQPW5P2PUBZPVK3LQTZTMNRFAVCNFSM6AAAAABMSYUJ22VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENRQGUYTINBSHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
WalterKolczynski-NOAA
requested changes
Aug 26, 2024
jobs/JGEFS_ATMOS_ACC
Outdated
Comment on lines
10
to
32
| #################################### | ||
| # obtain unique process id (pid) and make temp directory | ||
| #################################### | ||
|
|
||
| export pid=$$ | ||
| export date=${CDATE} | ||
| #yyyymmdd=`echo $date | cut -c1-8` | ||
| #echo `date` $0 `date -u` begin | ||
|
|
||
| #################################### | ||
| # File To Log Msgs | ||
| #################################### | ||
| export jlogfile=${jlogfile:-${COMROOT}/logs/jlogfiles/jlogfile.${job}.${pid}} | ||
| #echo $jlogfile | ||
|
|
||
| #################################### | ||
| # Determine Job Output Name on System | ||
| #################################### | ||
| export outid="LL${job}" | ||
| export jobid="${outid}.o${pid}" | ||
| export pgmout="OUTPUT.${pid}" | ||
| export pgmerr=errfile | ||
| #echo $outid,$jobid,$pgmout,$pgmerr |
There was a problem hiding this comment.
None of this is necessary. The jjob header takes care of what is needed, and most of the rest we don't use at all.
Suggested change
| #################################### | |
| # obtain unique process id (pid) and make temp directory | |
| #################################### | |
| export pid=$$ | |
| export date=${CDATE} | |
| #yyyymmdd=`echo $date | cut -c1-8` | |
| #echo `date` $0 `date -u` begin | |
| #################################### | |
| # File To Log Msgs | |
| #################################### | |
| export jlogfile=${jlogfile:-${COMROOT}/logs/jlogfiles/jlogfile.${job}.${pid}} | |
| #echo $jlogfile | |
| #################################### | |
| # Determine Job Output Name on System | |
| #################################### | |
| export outid="LL${job}" | |
| export jobid="${outid}.o${pid}" | |
| export pgmout="OUTPUT.${pid}" | |
| export pgmerr=errfile | |
| #echo $outid,$jobid,$pgmout,$pgmerr |
jobs/JGEFS_ATMOS_ACC
Outdated
| #################################### | ||
| # Specify Execution Areas | ||
| #################################### | ||
| export HOMEgefs=${HOMEgfs} |
There was a problem hiding this comment.
Please just use HOMEgfs instead of defining HOMEgefs.
jobs/JGEFS_ATMOS_ACC
Outdated
| # Specify Execution Areas | ||
| #################################### | ||
| export HOMEgefs=${HOMEgfs} | ||
| export EXECacc=${EXECacc:-${HOMEgefs}/exec} |
There was a problem hiding this comment.
We already have EXECgfs for this.
jobs/JGEFS_ATMOS_ACC
Outdated
| #################################### | ||
| export HOMEgefs=${HOMEgfs} | ||
| export EXECacc=${EXECacc:-${HOMEgefs}/exec} | ||
| export SORCacc=${SORCacc:-${HOMEgefs}/sorc/gefs_postacc.fd} |
There was a problem hiding this comment.
Why is this necessary
jobs/JGEFS_ATMOS_ACC
Outdated
| export HOMEgefs=${HOMEgfs} | ||
| export EXECacc=${EXECacc:-${HOMEgefs}/exec} | ||
| export SORCacc=${SORCacc:-${HOMEgefs}/sorc/gefs_postacc.fd} | ||
| export SCRIPTSens_acc=${HOMEgefs}/scripts |
scripts/gefs_atmos_f0306.sh
Outdated
There was a problem hiding this comment.
Please fix the indentation in this script.
Also, there is quite a bit of repetition. Consider consolidating it with loops.
scripts/gefs_atmos_f0306.sh
Outdated
|
|
||
| rm tmp | ||
| else | ||
| echo "${infile} does not exist" |
There was a problem hiding this comment.
Shouldn't this exit with an error?
workflow/applications/gefs.py
Outdated
| @@ -14,7 +14,7 @@ def _get_app_configs(self): | |||
| """ | |||
| Returns the config_files that are involved in gefs | |||
| """ | |||
| configs = ['stage_ic', 'fcst', 'atmos_products', 'arch'] | |||
| configs = ['stage_ic', 'fcst', 'repairf0306', 'atmos_products', 'arch'] | |||
There was a problem hiding this comment.
Can we call the new job (and associated scripts) repair_replay_acc or something similar?
Comment on lines
+172
to
+174
| def atmos_prod(self): | ||
| return self._atmosoceaniceprod('atmos') | ||
|
|
There was a problem hiding this comment.
This method is already defined.
Suggested change
| def atmos_prod(self): | |
| return self._atmosoceaniceprod('atmos') |
Comment on lines
+179
to
+186
| products_dict = {'atmos': {'config': 'atmos_products', | ||
| 'history_path_tmpl': 'COM_ATMOS_MASTER_TMPL', | ||
| 'history_file_tmpl': f'{self.run}[email protected]'}} | ||
| component_dict = products_dict['atmos'] | ||
| config = component_dict['config'] | ||
| history_path_tmpl = component_dict['history_path_tmpl'] | ||
| history_file_tmpl = component_dict['history_file_tmpl'] | ||
| history_path = self._template_to_rocoto_cycstring(self._base[history_path_tmpl], {'MEMDIR': 'mem#member#'}) |
There was a problem hiding this comment.
Suggested change
| products_dict = {'atmos': {'config': 'atmos_products', | |
| 'history_path_tmpl': 'COM_ATMOS_MASTER_TMPL', | |
| 'history_file_tmpl': f'{self.run}[email protected]'}} | |
| component_dict = products_dict['atmos'] | |
| config = component_dict['config'] | |
| history_path_tmpl = component_dict['history_path_tmpl'] | |
| history_file_tmpl = component_dict['history_file_tmpl'] | |
| history_path = self._template_to_rocoto_cycstring(self._base[history_path_tmpl], {'MEMDIR': 'mem#member#'}) | |
| history_path = self._template_to_rocoto_cycstring(self._base["COM_ATMOS_HISTORY_TMPL"], {'MEMDIR': 'mem#member#'}) |
| history_file_tmpl = component_dict['history_file_tmpl'] | ||
| history_path = self._template_to_rocoto_cycstring(self._base[history_path_tmpl], {'MEMDIR': 'mem#member#'}) | ||
|
|
||
| data = f'{history_path}/{history_file_tmpl}' |
There was a problem hiding this comment.
Suggested change
| data = f'{history_path}/{history_file_tmpl}' | |
| data = f'{history_path}/{self.run}[email protected]' |
EricSinsky-NOAA
changed the base branch from
develop
to
feature/gefs_reforecast
|
The target has been changed from |
|
@HongGuan-NOAA if/when this is ready for review, please take it off of draft and we will review it. |
|
Walter,
I have made the modifications based on your and Eric's suggestions. You can
review it. But I am not able to build the code with the g2_ver=3.4.5.
This is another PR. I can build the code with the older version (3.4.1).
Could I use the old version to build the code? If it's OK, let me know
where I should make modifications for the change. Thanks!
Hong
…On Wed, Sep 18, 2024 at 3:01 PM Walter Kolczynski - NOAA < ***@***.***> wrote:
@HongGuan-NOAA <https://github.com/HongGuan-NOAA> if/when this is ready
for review, please take it off of draft and we will review it.
—
Reply to this email directly, view it on GitHub
<#2838 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVLBRUV4BU7AWAKP73Q4XRTZXHEZFAVCNFSM6AAAAABMSYUJ22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJZGIYDCNBRGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds the repair task for correcting the F03 and F06 ave/acc/min/max variables.
Type of change
Change characteristics
How has this been tested?
This has been tested in WCOSS2.
Checklist