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

Fb_global_unstr #335

Merged
merged 28 commits into from
Mar 22, 2021
Merged

Fb_global_unstr #335

merged 28 commits into from
Mar 22, 2021

Conversation

aliabdolali
Copy link
Contributor

Pull Request Summary

This pull request addresses the periodicity fix for unstructured grids with global coverage, so energy can transfer at the two ends of the mesh. This PR solves the problem for two parallelizations implemented in WW3 for unstructured grids (Card-Deck and Domain Decomposition-PDLIB).

Description

  • The connectivity between elements is checked to find elements that connect two ends of the mesh with a longitudinal difference of ~360 degrees and lets energy transfer between them.
    Corrects calculation of element areas and edge lengths across the
    dateline in w3triamd.ftn
  • Fixes calculation to determine whether a point is inside an element that spans the dateline in w3triamd.ftn
  • Corrects calculation of element centers and corners across dateline in wmscrpmd.ftn
  • Write out SCRIP file in netCDF format at the end of ww3_grid (for creating offline remaping files). This required adding netCDF support to ww3_grid
  • Also includes minor fix for the /O7a switch in w3iopomd.ftn
  • A new regression test is added to check this enhancement (ww3_tp2.21). The inputs (mesh, wind forcing, UOST.in files) are added to the tar file on FTP server.

Issue(s) addressed

Developers: @sbrus89, @menta78 and @aliabdolali

Testing

  • regtests/ww3_tp2.21 is added for a global unstructured grid with both card deck and domain decomposition.
  • The matrix_ncep has been run on Hera machine using intel/18.0.5.274 impi/2018.0.4 netcdf/4.6.1.
  • Here is the summary output of matrix.comp on NOAA HPC (Hera):
    MatrixDiff.zip
  • expected changes in the outputs in mww3_test_02, mww3_test_03, mww3_test_04, mww3_test_06, ww3_tp2.17 ust in grid stdout for the nodes sign check (SI nbPlus,nbMinus,nbZero)
  • mww3_test_03, mww3_test_07, ww3_tp2.10, ww3_tp2.18 are expected non-identical tests.

UKMO-lsampson and others added 23 commits July 22, 2020 11:44
to ensure they comply with the limits of the nameslist.
Changes to add support to 360-day and 365-day (no leap year) calendar - see ticket #209
  * Additional CALTYPE namelist parameter in MISC section
  * New ww3_tc1 regtest.
* Updated ww3_bound and ww3_bounc to handle model grids formulated on a rotated pole.
* Manual and nml/inp files to updated clarify that ww3_bound/ww3_bounc only accept input spectra formulated on a standard pole grid.
Updates to allow a coupling time step that is different from the model time step. 
* Includes new regtest (in ww3_tp2.14) for non-default oasis time step.
* ww3_tp2.14 regtest added to matrix.base.
 - Corrects calculation of element areas and edge lengths across the
   dateline in w3triamd.ftn

 - Fixes calculation to determine whether a point is inside an element
   that spans the dateline in w3triamd.ftn

 - Corrects calculation of element centers and corners across dateline
   in wmscrpmd.ftn

 - Write out SCRIP file in netCDF format at the end of ww3_grid
   (for creating offline remaping files). This required adding netCDF support
   to ww3_grid

 - Also includes minor fix for the /O7a switch in w3iopomd.ftn
…ge from global grids from min(longitude)=-180, max(longitude)=180 to \delta(longitude)=360 degrees and added the regression test ww3_tp2.21
PDLIB/yowpdlibmain.ftn: fix to handle global meshes
@aliabdolali aliabdolali added the enhancement New feature or request label Mar 18, 2021
reg_programs="ww3_grid"
reg_programs="$reg_programs ww3_strt"
# reg_programs="ww3_grid"
# reg_programs="$reg_programs ww3_strt"
Copy link
Collaborator

Choose a reason for hiding this comment

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

clean up commented out text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -154,6 +155,7 @@
cdf_programs="$cdf_programs ww3_ounp"
cdf_programs="$cdf_programs ww3_bounc"
cdf_programs="$cdf_programs ww3_trnc"
cdf_programs="$cdf_programs ww3_grid"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there an option that says if ww3_grid should be a netcdf program or not? The way this currently is, you are now requiring netcdf for ww3_grid, which essentially is now requiring netcdf as a library, no? While I know there are ongoing discussions of integrating more netcdf, is there a way to make this optional or is it always essential now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JessicaMeixner-NOAA It is required when SCRIPNC is in the switch.
From a communication with @sbrus89:
The scrip.nc can be used with ESMF_regrid_gen_weights (or similar). I added this to aid in the creation of coupler mapping files for unstructured grids for E3SM. I wasn't sure if this was something that would be useful to include in the PR or not, so I decided to leave it in just in case. We can certainly leave it out if you think that would be better.

So, as Steven suggested, I can exclude SCRIP related developments, and we no longer need netcdf for ww3_grid, but I thought it might be helpful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are all of the netcdf SCRIP related updates behind a switch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I don't want to do, is require everyone even someone who is using all ascii files to now have a netcdf dependency. If we move all output to netcdf, then yes all programs will have netcdf dependency and it will be a requirement. Even in the make_makefile script, this looks like an option, not a requirement, so when deciding if ww3_grid is a netcdf or regular program, shouldn't this also be an option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized it when I was checking mww3_test_04 with switch_PR3_UQ_MPI_SCRIPNC, so I had to modify the make_makefile.sh, the rest of tests went well without netcdf dependency, so I think these are behind SCRIPNC if I am not wrong.

Copy link
Collaborator

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 moved into the if/else section starting on line 172: https://github.com/aliabdolali/WW3/blob/FB_Global_Unstr/model/bin/w3_make#L172

If it's scripnc then add ww3_grid to netcdf programs, if not, it should be a regular program.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JessicaMeixner-NOAA Thanks, it was a very helpful hint, I made the changes and ran the regtests again, all in good shape. Could you approve the review?

@JessicaMeixner-NOAA
Copy link
Collaborator

@aliabdolali thanks for updating the makefile. I'll continue the rest of my review now. Hopefully it'll be done by Monday.

@JessicaMeixner-NOAA
Copy link
Collaborator

@aliabdolali I'm okay with these changes, but I don't understand why this PR exists and why this was not included in PR #303 instead? Shouldn't anything here have instead been pushed to that PR? Why doesn't the target of this PR get changed to the branch used in PR#303. Once that is done, we can re-run regtests and it looks like the updates are good to go.

@aliabdolali
Copy link
Contributor Author

@aliabdolali I'm okay with these changes, but I don't understand why this PR exists and why this was not included in PR #303 instead? Shouldn't anything here have instead been pushed to that PR? Why doesn't the target of this PR get changed to the branch used in PR#303. Once that is done, we can re-run regtests and it looks like the updates are good to go.

@JessicaMeixner-NOAA This one is PR #303 plus works from Lorenzo on PDLIB, so he was not able to push his changes to Steven PR and I made it on my own fork. We can continue with this one and merge as it has periodicity fixes for both Card Deck and Domain Decomposition. I'll check with Steven if he has any objections.

@JessicaMeixner-NOAA
Copy link
Collaborator

Couldn't push to his branch or make a PR there? I thought anyone could make PRs to others branches? Please check with @sbrus89 and let me know.

@sbrus89
Copy link
Collaborator

sbrus89 commented Mar 19, 2021

@aliabdolali and @JessicaMeixner-NOAA, I'm okay with whatever you guys think works best. Since Ali already has the changes merged here, I'm perfectly fine with going ahead with #335 in place of #303.

@sbrus89
Copy link
Collaborator

sbrus89 commented Mar 19, 2021

I had not run any PDLIB cases, so I'm glad that bug got identified and addressed.

@aliabdolali
Copy link
Contributor Author

I had not run any PDLIB cases, so I'm glad that bug got identified and addressed.

Thanks @sbrus89 All in good shape with your development, and all types of options for unstructured are now functioning well for global meshes.

@aliabdolali
Copy link
Contributor Author

@JessicaMeixner-NOAA
I reran the test, it is similar to the previous comparison.
MatrixDiffMarch20.zip

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

Successfully merging this pull request may close these issues.

Periodicity fix for global unstructured grids
7 participants