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.

Sign up for GitHub

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

Jump to bottom

Fb smc datadim #753

Merged
merged 5 commits into from
Sep 2, 2022
Merged

Fb smc datadim #753

merged 5 commits into from
Sep 2, 2022

Conversation

UKMO-lsampson
Copy link
Collaborator

@UKMO-lsampson UKMO-lsampson commented Aug 5, 2022 •
edited
Loading

Pull Request Summary

Implement the removal of some redundant indices that are currently used in the cell (IJKCel) and face arrays (IJKUFc, IJKVFc) of the SMC propagation module.

Description

The current array accesses that are used for the SMC propagation are assigning some components that are never called. By limiting the index, we can reduce the memory usage while maintaining the ability to use the current input file and input arrays.

This does not fix any bug as the program still processes accordingly, but rather focuses on improving the memory usage for the SMC grid, and hence is referred to as a feature branch.

The regression tests should produce a difference when the mod_def.ww3 is produced using a SMC grid. This is because we have changed the declaration of the IJKCel array which is written to and read from mod_def.ww3.

Reviewers (to be requested):

  • @ukmo-jianguo-li JianGuo-Li [Met Office]
  • Ali Abdolali [NOAA]
  • Chris Bunney [Met Office]

Issue(s) addressed

Commit Message

Implement the removal of some redundant indices that are currently used in the cell (IJKCel) and face arrays (IJKUFc, IJKVFc) of the SMC propagation module.

Check list

Testing

  • How were these changes tested? Regtests
  • Are the changes covered by regression tests? Yes, the SMC grid is used by a few regtests.
  • Have the matrix regression tests been run (if yes, please note HPC and compiler)? Yes. Met Office Cray XC40 system, XCE. ftn compiler.
  • Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.) The mod_def.ww3 file for any smc grid regtest will be different as the definition of IJKCel has changed.
  • Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):

Regtest shows known differences, and changes due to mod_def.ww3:

**********************************************************************
********************* non-identical cases ****************************
**********************************************************************
mww3_test_03/./work_PR3_UQ_MPI_d2                     (10 files differ)
mww3_test_03/./work_PR2_UNO_MPI_e                     (1 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c                     (9 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2                     (11 files differ)
mww3_test_03/./work_PR2_UQ_MPI_e                     (1 files differ)
mww3_test_03/./work_PR2_UNO_MPI_d2                     (10 files differ)
mww3_test_03/./work_PR1_MPI_d2                     (14 files differ)
mww3_test_03/./work_PR1_MPI_e                     (1 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c                     (8 files differ)
mww3_test_03/./work_PR3_UNO_MPI_e                     (1 files differ)
mww3_test_03/./work_PR3_UQ_MPI_e                     (1 files differ)
mww3_test_03/./work_PR2_UQ_MPI_d2                     (9 files differ)
ww3_ta1/./work_UPD0F_U                     (0 files differ)
ww3_tp2.10/./work_MPI_OMPH                     (1 files differ)
     mod_def.ww3 differ (binary)

ww3_tp2.10/./work_MPI                     (1 files differ)
     mod_def.ww3 differ (binary)

ww3_tp2.14/./work_OASACM5                     (0 files differ)
ww3_tp2.14/./work_OASACM                     (1 files differ)
     core differ (binary)

ww3_tp2.14/./work_OASICM                     (1 files differ)
     core differ (binary)

ww3_tp2.14/./work_OASOCM                     (1 files differ)
     core differ (binary)

ww3_tp2.16/./work_MPI_OMPH                     (1 files differ)
     mod_def.ww3 differ (binary)

ww3_tp2.16/./work_MPI                     (1 files differ)
     mod_def.ww3 differ (binary)

@UKMO-lsampson UKMO-lsampson added the enhancement New feature or request label Aug 5, 2022
@ukmo-ccbunney
Copy link
Collaborator

Hi @UKMO-lsampson
Can you paste the results of the regtests summary in the PR description?
Thanks

@UKMO-lsampson
Copy link
Collaborator Author

Hi @UKMO-lsampson Can you paste the results of the regtests summary in the PR description? Thanks

Hi @ukmo-ccbunney, I am still in the process of running the regtests. I had to rerun the develop and comparison regtests as the NOAA and UKMO-wave develop updated. I will attach them ASAP, and change the PR to be ready for review once available.

@UKMO-lsampson
Copy link
Collaborator Author

Hi @UKMO-lsampson Can you paste the results of the regtests summary in the PR description? Thanks

Hi @ukmo-ccbunney, I am still in the process of running the regtests. I had to rerun the develop and comparison regtests as the NOAA and UKMO-wave develop updated. I will attach them ASAP, and change the PR to be ready for review once available.

@ukmo-ccbunney I have run the regtests and compared them with the output from the head of develop. This PR has been updated according to the results, but I will need to continue to investigate the reasons for some differences.

@JessicaMeixner-NOAA
Copy link
Collaborator

@UKMO-lsampson these look like pretty familiar//expected output. See https://github.com/NOAA-EMC/WW3/wiki/How-to-use-matrix.comp-to-compare-regtests-with-develop#4-look-at-results and then an unresolved issue for the unstructured grid mod_defs here: #700

@UKMO-lsampson
Copy link
Collaborator Author

UKMO-lsampson commented Aug 11, 2022 •
edited
Loading

@UKMO-lsampson these look like pretty familiar//expected output. See https://github.com/NOAA-EMC/WW3/wiki/How-to-use-matrix.comp-to-compare-regtests-with-develop#4-look-at-results and then an unresolved issue for the unstructured grid mod_defs here: #700

@JessicaMeixner-NOAA thank you for looking at this PR. I think you're correct, I have rerun the regtests that contain mod_def and core differences, with the same source code and this is still reporting differences in the binary files.

@UKMO-lsampson UKMO-lsampson marked this pull request as ready for review August 12, 2022 13:26
@ukmo-ccbunney
Copy link
Collaborator

@ukmo-jianguo-li Would you be able to do a quick review of this change please? Thanks.

@ukmo-jianguo-li
Copy link
Collaborator

I have viewed Lewis' changes and they look fine to me. Although some more columns in the face array could be removed from memory as I originally suggested, it could be left for later update. As this is Lewis' first pull request for the WW3 model, I think it is better to keep it as simple as possible and get it merged as quick as possible.

@ukmo-jianguo-li
Copy link
Collaborator

Have forgot how to submit the review approval. The system seems not taking my comment as approval.

@ukmo-ccbunney
Copy link
Collaborator

ukmo-ccbunney commented Aug 31, 2022 •
edited
Loading

Have forgot how to submit the review approval. The system seems not taking my comment as approval.

Thanks @ukmo-jianguo-li - we could not add you explicitly as a reviewer, so your approval in the comments is fine.
Edit - I think you need to be added as a "collaborator" to the NOAA WW3 repo to allow you to be a reviewer. @aliabdolali - can you do this for @ukmo-jianguo-li please? Thanks.

ukmo-ccbunney
ukmo-ccbunney approved these changes Aug 31, 2022
View reviewed changes
Copy link
Collaborator

@ukmo-ccbunney ukmo-ccbunney left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I've run the regression tests and get the following differences (ignoring the known differences in mww3_test_03):

mww3_test_09/./work_MPI                     (3 files differ)
ww3_tp2.10/./work_MPI                     (1 files differ)
ww3_tp2.10/./work_MPI_OMPH                     (1 files differ)
ww3_tp2.14/./work_OASACM6                     (1 files differ)
ww3_tp2.16/./work_MPI                     (1 files differ)
ww3_tp2.16/./work_MPI_OMPH                     (1 files differ)

In each case the difference is in the mod_def.* files, as expected (the IJKCel array is now a different size).

I'm happy to approve this PR.

@JessicaMeixner-NOAA
Copy link
Collaborator

I will run the regtests here one more time just as an extra sanity check. It might take a day as our queues are a little slow, but then we should have this merged tomorrow or Friday.

@aliabdolali
Copy link
Contributor

Have forgot how to submit the review approval. The system seems not taking my comment as approval.

Thanks @ukmo-jianguo-li - we could not add you explicitly as a reviewer, so your approval in the comments is fine. Edit - I think you need to be added as a "collaborator" to the NOAA WW3 repo to allow you to be a reviewer. @aliabdolali - can you do this for @ukmo-jianguo-li please? Thanks.

I just did it again, he was invited but never accepted the invitation so it was expired.

@JessicaMeixner-NOAA
Copy link
Collaborator

Here's my full list of cases that are different:

mww3_test_03/./work_PR2_UNO_MPI_d2 (12 files differ)
mww3_test_03/./work_PR1_MPI_d2 (16 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c (14 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c (15 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2 (12 files differ)
mww3_test_03/./work_PR2_UQ_MPI_d2 (15 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2 (15 files differ)
mww3_test_09/./work_MPI (3 files differ)
ww3_tp2.10/./work_MPI (1 files differ)
ww3_tp2.10/./work_MPI_OMPH (7 files differ)
ww3_tp2.14/./work_OASACM6 (1 files differ)
ww3_tp2.16/./work_MPI (1 files differ)
ww3_tp2.16/./work_MPI_OMPH (5 files differ)
ww3_tp2.17/./work_a (1 files differ)
ww3_tp2.17/./work_c (1 files differ)
ww3_tp2.17/./work_b (1 files differ)
ww3_tp2.6/./work_ST0 (1 files differ)
ww3_tp2.6/./work_ST4 (1 files differ)
ww3_tp2.6/./work_pdlib (1 files differ)
ww3_ufs1.3/./work_a (1 files differ)

These are known-issue files:

mww3_test_03/./work_PR2_UNO_MPI_d2 (12 files differ)
mww3_test_03/./work_PR1_MPI_d2 (16 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c (14 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c (15 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2 (12 files differ)
mww3_test_03/./work_PR2_UQ_MPI_d2 (15 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2 (15 files differ)
ww3_tp2.16/./work_MPI (1 files differ)
ww3_tp2.16/./work_MPI_OMPH (5 files differ)
ww3_tp2.17/./work_a (1 files differ)
ww3_tp2.17/./work_c (1 files differ)
ww3_tp2.17/./work_b (1 files differ)
ww3_tp2.6/./work_ST0 (1 files differ)
ww3_tp2.6/./work_ST4 (1 files differ)
ww3_tp2.6/./work_pdlib (1 files differ)
ww3_ufs1.3/./work_a (1 files differ)

These are known to change from this PR:
mww3_test_09/./work_MPI (3 files differ)
ww3_tp2.10/./work_MPI (1 files differ)
ww3_tp2.10/./work_MPI_OMPH (7 files differ) -- 6 is normal, extra 1 for mod_def
ww3_tp2.14/./work_OASACM6 (1 files differ)

@JessicaMeixner-NOAA
Copy link
Collaborator

matrixCompSummary.txt
matrixDiff.txt
matrixCompFull.txt

@JessicaMeixner-NOAA JessicaMeixner-NOAA merged commit e878827 into NOAA-EMC:develop Sep 2, 2022
@ukmo-ccbunney ukmo-ccbunney deleted the fb_smc_datadim branch May 12, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ukmo-ccbunney ukmo-ccbunney ukmo-ccbunney approved these changes

@JessicaMeixner-NOAA JessicaMeixner-NOAA JessicaMeixner-NOAA approved these changes

@aliabdolali aliabdolali Awaiting requested review from aliabdolali

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@UKMO-lsampson @ukmo-ccbunney @JessicaMeixner-NOAA @ukmo-jianguo-li @aliabdolali