Skip to content
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

Optimized ww3_outp for the netcdf point output #1365

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

AliS-Noaa
Copy link
Collaborator

@AliS-Noaa AliS-Noaa commented Feb 7, 2025

Pull Request Summary

Optimized the ww3_outp for netcdf point output

Description

Following the #1355 and @dkokron work, the ww3_outp in improved to read per time step NetCDF output and create different point output.

Issue(s) addressed

Addressing issue #876

Commit Message

Optimized ww3_outp for the netcdf point output

Check list

Testing

  • How were these changes tested? Full regression tests ran, please see the matrix.
  • Are the changes covered by regression tests? (If not, why? Do new tests need to be added?) regression test, ww3_ufs1.1 is modified to test the new ww3_outp implementation for the global-workflow.
  • Have the matrix regression tests been run (if yes, please note HPC and compiler)?
  • Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.) For ww3_ufs1.1 regression test, 3 different ww3_outp.inp are added to produce .spec, .bull, and .cbull point outputs similar to global-workflow.
  • Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):

********************* non-identical cases ***************************


mww3_test_03/./work_PR2_UQ_MPI_d2 (16 files differ)
mww3_test_03/./work_PR2_UNO_MPI_d2 (9 files differ)
mww3_test_03/./work_PR2_UQ_MPI_e (1 files differ)
mww3_test_03/./work_PR3_UNO_MPI_e (1 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c (17 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2 (15 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c (9 files differ)
mww3_test_03/./work_PR1_MPI_d2 (9 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2 (16 files differ)
mww3_test_03/./work_PR1_MPI_e (1 files differ)
mww3_test_03/./work_PR3_UNO_MPI_e_c (1 files differ)
mww3_test_03/./work_PR3_UQ_MPI_e (1 files differ)
mww3_test_03/./work_PR2_UNO_MPI_e (1 files differ)
mww3_test_09/./work_MPI_ASCII (0 files differ)
ww3_tp2.10/./work_MPI_OMPH (7 files differ)
ww3_tp2.16/./work_MPI_OMPH (4 files differ)
ww3_tp2.6/./work_ST4_ASCII (0 files differ)
ww3_ufs1.1/./work_unstr_b (1 files differ)
ww3_ufs1.1/./work_unstr_a (1 files differ)
ww3_ufs1.1/./work_unstr_c (1 files differ)
ww3_ufs1.3/./work_a (3 files differ)
matrixCompFull.txt
matrixCompSummary.txt
matrixDiff.txt

@AliS-Noaa AliS-Noaa marked this pull request as draft February 7, 2025 20:33
Copy link
Collaborator

@JessicaMeixner-NOAA JessicaMeixner-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all of your efforts for bringing theses changes into the develop branch from production v16 branch.

I've left a few comments in the code. We either need to create an issue to add the two new variables for reading the nml file or we need to add that to the code here now. Additionally as mentioned in the code, we need to describe this new variable in the example inp files. Lastly, a regression test for this new feature needs to be added. I will help with that and make a PR to your branch by Monday.

@@ -2674,7 +2839,7 @@ SUBROUTINE W3EXPO
' Hs Tp dir |'/ &
' | hour | (m) - - | (m) (s) (d) |', &
' (m) (s) (d) |', &
' (m) (s) (d) |', &
' (m) (s) (d) |', &
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra space at end of line here.

ELSE
filename = FNMPRE(:LEN_TRIM(FNMPRE))//'out_pnt.'//FILEXT(:LEN_TRIM(FILEXT))//'.nc'
END IF

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AliS-Noaa - Particularly because the code will fail without a very informative error, before trying to open the file, adding in a check to see if the file exists and if it does not, providing an informative error statement before existing would be helpful. Please let me know if you need any assistance adding this.

…g the prefix to W3IOPON/W3IOPON_READ;some more cleaning
$
20210401 000000 3600. 100
20210401 000000 3600. 25 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the permissions changing on this file. Also, please add a prefix here so that we can test that feature.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a second ww3_outp to do both bull/cbull and spec files to test both features?

$ flags for spectrum and source terms (C*21, 3I, 6L)
$ - Bin frequencies in Hz for all bins.
$ - Bin frequenies in Hz for all bins.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling frequenies- > frequencies

$
$ The transfer file contains records with the following contents.
$
$ - File ID in quotes, number of frequencies, directions and points,
$ - File ID in quotes, nubmer of frequencies, directions and points,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling nubmer -> number


END IF

!Variables read based on time (IPASS):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!Variables read based on time (IPASS):
->
!Variables read based on time (itime):

END IF

END IF
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra space here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JessicaMeixner-NOAA This is done and its ready for review.

@AliS-Noaa AliS-Noaa marked this pull request as ready for review February 11, 2025 15:37
@JessicaMeixner-NOAA
Copy link
Collaborator

@AliS-Noaa - Thanks for the last update you made. My regression tests before your update were as expected. We'll hold off on a final round of testing until we've finished testing in g-w. In the meantime, please don't forget to fill out the testing portion of the PR when you have finalized things.

@AliS-Noaa
Copy link
Collaborator Author

@JessicaMeixner-NOAA I tested this with updated G-W and it is ready for the review.

@JessicaMeixner-NOAA
Copy link
Collaborator

Thank you @AliS-Noaa - Starting review and testing now!! Thanks for all of this! It's really appreciated.

@JessicaMeixner-NOAA
Copy link
Collaborator

@AliS-Noaa - In operations when I un-tar the spec files or other tar-balls, I have:
gfswave.NAME.spec or gadswave.NAME.spec
however, I do not get this in this branch.

While we can leave off the prefix for the YYYYMMDD.HHMMSS.out_pnt.ww3 file which will simplify for example the regression test, I think this needs to be re-added to have correctly named output to match what we are obtaining in operations. I think it'll be more efficient to have these files output with the correct name versus having to rename them all before tarring.

@AliS-Noaa
Copy link
Collaborator Author

@AliS-Noaa - In operations when I un-tar the spec files or other tar-balls, I have: gfswave.NAME.spec or gadswave.NAME.spec however, I do not get this in this branch.

While we can leave off the prefix for the YYYYMMDD.HHMMSS.out_pnt.ww3 file which will simplify for example the regression test, I think this needs to be re-added to have correctly named output to match what we are obtaining in operations. I think it'll be more efficient to have these files output with the correct name versus having to rename them all before tarring.

@JessicaMeixner-NOAA so do you need to bring back the optional prefix to the code and also the g-w workflow script? I can work on it now.

@JessicaMeixner-NOAA
Copy link
Collaborator

@AliS-Noaa - In operations when I un-tar the spec files or other tar-balls, I have: gfswave.NAME.spec or gadswave.NAME.spec however, I do not get this in this branch.
While we can leave off the prefix for the YYYYMMDD.HHMMSS.out_pnt.ww3 file which will simplify for example the regression test, I think this needs to be re-added to have correctly named output to match what we are obtaining in operations. I think it'll be more efficient to have these files output with the correct name versus having to rename them all before tarring.

@JessicaMeixner-NOAA so do you need to bring back the optional prefix to the code and also the g-w workflow script? I can work on it now.

Yes, thank you! Please let me know when you're ready for re-review

@JessicaMeixner-NOAA
Copy link
Collaborator

@AliS-Noaa - I'm working to determine why I'm getting tiny differences in the output between a global-workflow case using your branch merged to dev-ufs-weather-model versus a "baseline" case. I'll post more information when I have it. The WW3 regression tests themselves pass. I hope to have more information this afternoon or tomorrow morning.

@JessicaMeixner-NOAA
Copy link
Collaborator

@AliS-Noaa and I have been chatting offline. The remaining issue here is when running in the full workflow, we expect to get the same answers however everything is matching except for the partition number in the bull file. This is being investigated before this PR is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants