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
Update wave jobs to use COMIN/COMOUT #2643
Update wave jobs to use COMIN/COMOUT #2643
Changes from 9 commits
63c47ab
74370c4
47f873c
8427e46
257ec6b
5bafc6b
1861a77
b8e0c00
8432581
3118937
31082aa
a9a0258
94a9688
cdb0e82
883fa7c
2a81ae8
87e2492
4d27a6c
cb1764c
1241ede
b0c7bad
4b2f7ce
8f65499
cc4b281
d17d688
7438915
226e112
2acd4cc
52bfa61
31e3e88
6be0cef
4ce3fe5
96e3b42
a6b36c7
a3fcbe1
4a5fef0
0131597
526c64c
fc6d0c2
e125237
3351699
0c53d4b
5b35906
cde9194
1186cad
cf666ea
cfc4f78
b224d62
a549600
03eba4e
416b524
452fcd9
7401664
acbf9c9
0fed287
0f91c5c
7922af7
eb5fe34
9ecbfd5
0134119
abef5ef
401bbba
ccc0718
0474c12
aff4814
6214778
26ba301
11a7937
67417a3
63904c2
fbbd643
e17eec7
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.
what is being written out in
COMOUT_WAVE_HISTORY
from this job?This should be a
COMIN
variable, if I understand this job correctly.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.
The logic was derived from here.
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.
@aerorahul What is the verdict here?
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.
Where are the inputs derived from in this job?
Those should be labeled
COMIN_
.Where are the outputs from this job placed under?
Those should be labeled as
COMOUT_
.Without looking in what the job is doing and placing data, I don't think I can state definitively. I can definitely say though,
WAVE_HISTORY
should not be aCOMOUT_
variable from this job.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.
Updated.
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.
@HenryWinterbottom-NOAA I still do not think we need a COMOUT_WAVE_HISTORY. If that means that we remove it from line 18 as well, I think that's okay.
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.
Jessica is correct. I did an audit of these last week,
COMOUT_WAVE_HISTORY
should not be needed.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.
@HenryWinterbottom-NOAA COMOUT_WAVE_HISTORY should be removed here.
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.
@JessicaMeixner-NOAA Thank you. I missed this in the thread. Fixed.
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.
Why is this a
COMOUT
? Is this job writing out to this directory?What are the inputs to this job?
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 https://github.com/HenryWinterbottom-NOAA/global-workflow/blob/526c64ccd997e316a6eba781fd14cacaf41ba897/jobs/JGLOBAL_WAVE_POST_BNDPNTBLL#L22.
If
mkdir
is being used, is that not creating (e.g., outputting) a directory tree?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 just did a quick check and I don't see anywhere that we're writing out to the wave history directory - although it is a bit odd considering the line for creating the directory. I assume that's a mistake from copy/pasting from something else and was not properly cleaned up.
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.
Thank you for the clarification, @JessicaMeixner-NOAA. In that case, all
COMOUT
should then beCOMIN
as noted by @aerorahul. Correct?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 COMOUT_WAVE_HISTORY -> COMIN_WAVE_HISTORY
I'm checking on COMIN_WAVE_STATION I think this should be COMOUT... but i'll have to look .
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.
Yes, WAVE_STATION is where output goes: https://github.com/NOAA-EMC/global-workflow/blob/develop/ush/wave_tar.sh#L185
So COMIN_WAVE_STATION -> COMOUT_WAVE_STATION
WAVE_PREP and WAVE_HISTORY are where inputs are coming from.
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.
Thanks @JessicaMeixner-NOAA
That was also my understanding.
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.
@WalterKolczynski-NOAA Can you please confirm if the suggestions, above, are valid? These seem to conflict with https://github.com/HenryWinterbottom-NOAA/global-workflow/blob/feature/gwdev_issue_2451/ush/forecast_postdet.sh
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.
@HenryWinterbottom-NOAA the forecast model writes to this location, this post job reads from this location.
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.
Again, no need for COMOUT_WAVE_HISTORY, unless I'm missing something.
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.
@HenryWinterbottom-NOAA - I think this is still unresolved and should not be here.
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.
This was also resolved.