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

Improve oasis grid writing #1354

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

Conversation

ukmo-juan-castillo
Copy link
Collaborator

@ukmo-juan-castillo ukmo-juan-castillo commented Jan 24, 2025

Pull Request Summary

Rewrite the cpl_oasis_grid subroutine for better performance and to use OASIS and SCRIP standards.

Description

This is just a change providing a small performance gain in the code (mainly substituting three identical double loops by a single one), using the SCRIP convention to write grid corners, and the oasis convention to declare the grid mask. It should not change answers, but the grid.nc file generated by OASIS will change due to a rearrangement of data in regular grids and the change of mask values for SMC grids.

The reviewer is encouraged to look at the OASIS and SCRIP documentation.
Possible label that could be applied could be enhancement.

Issue(s) addressed

Commit Message

Rewrite the cpl_oasis_grid subroutine for better performance and to use OASIS and SCRIP standards.

Check list

Testing

  • How were these changes tested?
    Regression tests
  • Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)
    Yes, they are covered by the tp2.14 tests
  • Have the matrix regression tests been run (if yes, please note HPC and compiler)?
    Yes
  • Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.)
    The change only affect tests tp2.14. The only change in results has to do with the changes in the grid.nc OASIS output file discussed above.
  • Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):

@JessicaMeixner-NOAA
Copy link
Collaborator

Thanks for the PR @ukmo-juan-castillo - Ill start the regression tests from the NCEP side. Since this code is mostly with the OASIS coupling, I'm also going to ask @mickaelaccensi to review as he's more of an expert than I am.

@JessicaMeixner-NOAA
Copy link
Collaborator

@ukmo-juan-castillo Do you have any regression tests information about what might be expected to be changed answer wise? I know that @ukmo-ccbunney used to run those, so I don't know if you or others were running those as well?

@ukmo-juan-castillo
Copy link
Collaborator Author

Regression test information:

matrixCompSummary.txt-ww3_tp2.14.txt

@ukmo-juan-castillo
Copy link
Collaborator Author

@ukmo-juan-castillo Do you have any regression tests information about what might be expected to be changed answer wise? I know that @ukmo-ccbunney used to run those, so I don't know if you or others were running those as well?

Thank you, it is some time since I opened a PR but @ukmo-ccbunney helped me to refresh the procedure. I run the regtests myself, my old scripts were still working and it was straightforward. I was also thinking of @mickaelaccensi as a reviewer.

Copy link
Collaborator

@mickaelaccensi mickaelaccensi left a comment

Choose a reason for hiding this comment

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

I approve to write the mask as in scrip convention since it is already how it is created when reading MASPTA. ok for the order of the cell corners

@JessicaMeixner-NOAA
Copy link
Collaborator

@ukmo-juan-castillo - Okay to make sure we do proper testing on our end - we're trying to resovle a bug #1350

That being said, everything I have tested and looked at so far is okay - and given @mickaelaccensi review this should be merged in a couple of days. If that delay is an issue, let me know and I'll see what we can do.

@ukmo-juan-castillo
Copy link
Collaborator Author

@ukmo-juan-castillo - Okay to make sure we do proper testing on our end - we're trying to resovle a bug #1350

That being said, everything I have tested and looked at so far is okay - and given @mickaelaccensi review this should be merged in a couple of days. If that delay is an issue, let me know and I'll see what we can do.

Thank you Jessica. There is no urgency in having this merged, in fact it is something I wanted to do for a long time but never had the time. I will be creating more issues in the near future with similar code changes.

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.

Improvement of oasis grid write routine
3 participants