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.
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
Splits AWIPS jobs into seperate tasks #2094
Splits AWIPS jobs into seperate tasks #2094
Changes from 20 commits
10859c2
9df2539
2a560e3
e26aff3
3ab096a
8c42c79
76d91e0
f626d91
afe078d
40f37ec
9297b1e
42dac7f
82c8f1b
3e9a24c
5c2b60a
76d4e8c
7b41f31
763c516
7e4e358
dd20e83
ea27c25
46bf631
920460b
df94d43
fb7fefb
3347e8e
993956c
78aef53
2977c86
9ba2287
a50f865
9cc697d
1066a72
e9c9209
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Wondering if these should be configurable settings in config instead of being hardcoded (these are historically like this so perhaps now is the time to refine 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.
I think this needs to be done, but is outside the scope of this PR.
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.
Same thoughts....perhaps these
fhmin
andfhmax
variables should be variables?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.
If it is past
FHMAX_GFS
, there are no more forecasts. Should this bebreak
?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.
See comment earlier about hardcoded values.
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.
@KateFriedman-NOAA I will wait for a consensus regarding hard-coded values.
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.
See comment earlier about hardcoded values.
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.
Do we want these hardcoded still or configurable (but still set to 240 for now)?
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.
I'd call this outside scope as well.
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.
See comment earlier about job names.
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.
Done.
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.
Not sure why
sh
is needed in these names? IMO, they should beawips_g2
andawips_20km
. The later is short forawips_20km_1p0deg
. It is fine to use the full name and be clear.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.
They were included to reflect the original
awips.sh
that called multiple j-job calls. For example,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.
I updated this to be consistent with @KateFriedman-NOAA's comment here.
This is the result for a GFS forecast-only 120-forecast using AWIPS (and no waves):