Netcdf parallel output extensions and bug fixes #1446
Conversation
kestonsmith-noaa
requested review from
JesseMeng-NOAA and
JessicaMeixner-NOAA
and removed request for
JesseMeng-NOAA
JessicaMeixner-NOAA
left a comment
JessicaMeixner-NOAA
left a comment
There was a problem hiding this comment.
@kestonsmith-noaa - This will need a UFS weather-model corresponding PR for this to be merged. In the meantime, I have two questions one is in the code and the other is if you can please point to the issue or place where you have documented the remaining issues that were not addressed in this PR and link it to this PR as well.
| ! Group 5 | ||
| if (vname .eq. 'USTX') call write_var2d(vname, ust (1:nseal_cpl)*asf(1:nseal_cpl), dir=cos(ustdir(1:nseal_cpl)), usemask='true') | ||
| if (vname .eq. 'USTY') call write_var2d(vname, ust (1:nseal_cpl)*asf(1:nseal_cpl), dir=sin(ustdir(1:nseal_cpl)), usemask='true') | ||
| if (vname .eq. 'USTX') call write_var2d(vname, ust (1:nsea)*asf(1:nsea), dir=cos(ustdir(1:nsea)), init0='false', global='true') |
There was a problem hiding this comment.
@kestonsmith-noaa - Looking at patterns here and everything else seems to be for (1:nseal_cpl) should this also be that way?
There was a problem hiding this comment.
I knew I had seen some issue w/ ust before, and it was this one #942. So ust/ustdir/asf are dimensioned like global variables. At the time of this issue, I was testing w/ the "old" netcdf interface, which bootstrapped on top of the existing w3iogomd---meaning it was writing serial netcdf from the same root task as the binary out put was using. It does look like in this case the fields, for whatever reason, are global like group 1.
|
@DeniseWorthen - I'm adding you as a reviewer on this PR. We have some time because the queue in UFS weather-model is backed up at the moment. @kestonsmith-noaa will also be creating a ufs-waether-model PR and enabling netcdf history output in at least one additional test. We are not going to fully move to netcdf for all output because for operations we still do not have a netcdf-> grib capability, so we'll be sticking to binary for now, however do hope to move to netcdf when we have the netcdf -> grib capability. Not every single variable what checked due to some needing additional physics or other parameters turned on to fully check that are not as commonly used. Our strategy for this will be to document those and move forward with these updates. Thank you so much for creating this capability and apologies it's taken us so long to do this careful check between binary and netcdf and lastly - thanks to @kestonsmith-noaa for doing this careful check!!. |
|
@JessicaMeixner-NOAA Thanks. I also was doing some minor fix-up of the wav_history to fix the stride=0 issue (ie, when using history_n/_option like CESM does) given the additional variables that have been added in the meantime. That branch is https://github.com/DeniseWorthen/WW3/tree/feature/fixstride0. I did glance at this PR briefly when it had been opened against the develop branch. I think we need to be careful in the understanding of writing global arrays and decomposed arrays. For the netcdf option, only decomposed arrays are written. Arrays which are global (ie, the forcing fields) are first 'decomposed' and then written in the write_varXd routines. I will make an effort to review this soon; I definitely have questions about the 'transpose' decomposition as well as the one you point out where the entire (nsea) array is being sent. |
|
I am not sure why UST[X,Y] behave differently than the other Group 5
variables, and perhaps the change I suggested is masking a different issue
for these fields. For USTX and USTY I checked a number of masking etc.
options and this treatment as global variables (i.e. like group 1) was the
configuration that created output that matched the binary output. I
will rerun those cases to confirm the behavior with the current
dev/ufs-weather-model and the proposed changes today. The
original symptom was spatial noise in the UST[X,Y] fields when output
directly as NetCDF.
…On Fri, Jun 6, 2025 at 9:47 AM Jessica Meixner ***@***.***> wrote:
***@***.**** commented on this pull request.
@kestonsmith-noaa <https://github.com/kestonsmith-noaa> - This will need
a UFS weather-model corresponding PR for this to be merged. In the
meantime, I have two questions one is in the code and the other is if you
can please point to the issue or place where you have documented the
remaining issues that were not addressed in this PR and link it to this PR
as well.
------------------------------
In model/src/wav_history_mod.F90
<#1446 (comment)>:
> @@ -372,8 +437,9 @@ subroutine write_history ( timen )
if(vname .eq. 'PNR') call write_var2d(vname, pnr (1:nseal_cpl) )
! Group 5
- if (vname .eq. 'USTX') call write_var2d(vname, ust (1:nseal_cpl)*asf(1:nseal_cpl), dir=cos(ustdir(1:nseal_cpl)), usemask='true')
- if (vname .eq. 'USTY') call write_var2d(vname, ust (1:nseal_cpl)*asf(1:nseal_cpl), dir=sin(ustdir(1:nseal_cpl)), usemask='true')
+ if (vname .eq. 'USTX') call write_var2d(vname, ust (1:nsea)*asf(1:nsea), dir=cos(ustdir(1:nsea)), init0='false', global='true')
@kestonsmith-noaa <https://github.com/kestonsmith-noaa> - Looking at
patterns here and everything else seems to be for (1:nseal_cpl) should this
also be that way?
—
Reply to this email directly, view it on GitHub
<#1446 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AZUY35ACJE6V6ZK4EYRR5AT3CGLYFAVCNFSM6AAAAAB6NK5HI6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSMBUHE3TQMRQGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@kestonsmith-noaa I think they behave differently because there are no "extended" arrays for the ust/ustdir/asf variables. For example, charn (another gr 5 variable), has an associated "extended" pointer (xcharn), which gets pointed to when you call w3xeta (so that the single proc can write it out in w3iogo). At least, that is how I understand how the binary output works. |
|
I've started a document to the remaining issues here:
https://docs.google.com/document/d/1bopX120xWwyNU-DxnSHGHajc8jMkRZ_SHdM4gIcgMRY/edit?tab=t.0.
This includes a list of fields not tested in our experiment, and
descriptions of the fields which are not bit-for-bit and links to relevant
issues. I need to add a better description of the experiment which I
should have later this evening:
Best, Keston
On Fri, Jun 6, 2025 at 11:03 AM Keston Smith - NOAA Affiliate <
***@***.***> wrote:
… I am not sure why UST[X,Y] behave differently than the other Group 5
variables, and perhaps the change I suggested is masking a different issue
for these fields. For USTX and USTY I checked a number of masking etc.
options and this treatment as global variables (i.e. like group 1) was the
configuration that created output that matched the binary output. I
will rerun those cases to confirm the behavior with the current
dev/ufs-weather-model and the proposed changes today. The
original symptom was spatial noise in the UST[X,Y] fields when output
directly as NetCDF.
On Fri, Jun 6, 2025 at 9:47 AM Jessica Meixner ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
>
> @kestonsmith-noaa <https://github.com/kestonsmith-noaa> - This will need
> a UFS weather-model corresponding PR for this to be merged. In the
> meantime, I have two questions one is in the code and the other is if you
> can please point to the issue or place where you have documented the
> remaining issues that were not addressed in this PR and link it to this PR
> as well.
> ------------------------------
>
> In model/src/wav_history_mod.F90
> <#1446 (comment)>:
>
> > @@ -372,8 +437,9 @@ subroutine write_history ( timen )
> if(vname .eq. 'PNR') call write_var2d(vname, pnr (1:nseal_cpl) )
>
> ! Group 5
> - if (vname .eq. 'USTX') call write_var2d(vname, ust (1:nseal_cpl)*asf(1:nseal_cpl), dir=cos(ustdir(1:nseal_cpl)), usemask='true')
> - if (vname .eq. 'USTY') call write_var2d(vname, ust (1:nseal_cpl)*asf(1:nseal_cpl), dir=sin(ustdir(1:nseal_cpl)), usemask='true')
> + if (vname .eq. 'USTX') call write_var2d(vname, ust (1:nsea)*asf(1:nsea), dir=cos(ustdir(1:nsea)), init0='false', global='true')
>
> @kestonsmith-noaa <https://github.com/kestonsmith-noaa> - Looking at
> patterns here and everything else seems to be for (1:nseal_cpl) should this
> also be that way?
>
> —
> Reply to this email directly, view it on GitHub
> <#1446 (review)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AZUY35ACJE6V6ZK4EYRR5AT3CGLYFAVCNFSM6AAAAAB6NK5HI6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSMBUHE3TQMRQGY>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
|
Thanks Denise, That is very helpful. I assume adding the "extended"
pointer is probably not worth the trouble for the output
performance benefits at this point. Best, Keston
…On Fri, Jun 6, 2025 at 2:00 PM Denise Worthen ***@***.***> wrote:
*DeniseWorthen* left a comment (NOAA-EMC/WW3#1446)
<#1446 (comment)>
@kestonsmith-noaa <https://github.com/kestonsmith-noaa> I think they
behave differently because there are no "extended" arrays for the
ust/ustdir/asf variables. For example, charn (another gr 5 variable), has
an associated "extended" pointer (xcharn), which gets pointed to when you
call w3xeta (so that the single proc can write it out in w3iogo). At least,
that is how I understand how the binary output works.
—
Reply to this email directly, view it on GitHub
<#1446 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AZUY35HYJOJ6EKT5KJHNY7T3CHJKFAVCNFSM6AAAAAB6NK5HI6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSNJQGAZDSMJWHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Just making a note here that the contents of this PR has moved to:#1476 |
4 participants
Pull Request Summary
Repair bugs in netcdf parallel output and extend netcdf output capacity beyond original implementation.
Description
Changes only affect coupled runs within ufs-weather-model at present. In coupled ufs-weather-model runs changes are expected to PDIR, UST, THS, PSI, CGE and DTDYN variables.
Issue(s) addressed
Addresses:
#1366
Also has relevance to:
#1401
Commit Message
Fix bugs in netcdf parallel output and adds support for wave number and frequency dimensions in history output.
Check list
Testing