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

Semi-implicit barotropic mode solver updates #5019

Merged
merged 5 commits into from
Jul 1, 2022
Merged

Conversation

hyungyukang
Copy link
Contributor

@hyungyukang hyungyukang commented Jun 13, 2022

This PR updates the semi-implicit barotropic mode solver :

  • Restructure and make codes simpler in the main time-stepping code
  • Make subroutines for the semi-implicit solver (preconditioining, matrix-vector multiplications, global reductions)
  • Provide the s-step conjugate gradient method when config_btr_si_partition_match_mode is turned on to provide a faster symmetric iterative solver
  • Small changes in stage 1 and 3 to match the split-explicit code

Below is PASS/FAIL test results:

  • With default options
    FAIL PEM_Ln9_D.T62_oQU240.GMPAS-IAF.cori-knl_intel (FAIL is expected for PEM tests.)
    PASS PET_Ln9.T62_oQU240.GMPAS-IAF.cori-knl_intel
    PASS SMS.T62_oEC60to30v3wLI.GMPAS-DIB-IAF-ISMF.cori-knl_intel
    PASS SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.cori-knl_intel

  • With config_btr_si_partition_match_mode=.true.
    PASS PEM_Ln9_D.T62_oQU240.GMPAS-IAF.cori-knl_intel
    PASS PET_Ln9.T62_oQU240.GMPAS-IAF.cori-knl_intel
    PASS SMS.T62_oEC60to30v3wLI.GMPAS-DIB-IAF-ISMF.cori-knl_intel
    PASS SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.cori-knl_intel

This PR updates the semi-implicit barotropic mode solver:
 - Restructure and make codes simpler in the main time-stepping code
 - Make subroutines for the semi-implicit solver
   (preconditioining, matrix-vector multiplications, global reductions)
 - Provide the s-step conjugate gradient method when
   'config_btr_si_partition_match_mode' is turned on to provide
   faster symmetric iterative solver
 - Small changes in stage 1 and 3 to match the split-explicit code
@hyungyukang hyungyukang changed the title Hkang/ocean/si update Semi-implicit barotropic mode solver updates Jun 13, 2022
@hyungyukang hyungyukang self-assigned this Jun 13, 2022
@hyungyukang hyungyukang requested a review from philipwjones June 13, 2022 18:50
@jonbob jonbob assigned jonbob and unassigned hyungyukang Jun 14, 2022
@jonbob
Copy link
Contributor

jonbob commented Jun 14, 2022

@hyungyukang - why is FAIL is expected for PEM tests?

@philipwjones
Copy link
Contributor

@jonbob If the config_btr_si_partition_match_mode flag is false, the implicit solver uses a more optimal preconditioner that depends on the partitioning so won't be bfb (and uses faster non-reproducible sums as well). When the flag is true, both reproducible sums and a reproducible preconditioner are used that will guarantee a passing PEM test but with reduced performance. This was done when the SI solver was first introduced so this behavior hasn't changed for this PR, other than using the newer repro sums.

@jonbob
Copy link
Contributor

jonbob commented Jun 14, 2022

Thanks @philipwjones - I appreciate the clarification, since I misread it and assumed the default settings meant generic E3SM settings which need to be BFB

@hyungyukang
Copy link
Contributor Author

@philipwjones , Thanks a lot for your detailed explanation! Nothing to add more.
@jonbob, Sorry for confusing. I should've added reasons for the fail-expected tests. Thanks.

Copy link
Contributor

@philipwjones philipwjones left a comment

Choose a reason for hiding this comment

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

Visual review of changes looks good.

Passes compass PR test when option is off so is bfb as a stealth feature. Unfortunately, I forgot to build compass with LAPACK on so wasn't able to test with the option on.

It runs in a QU240 case on Summit with CPU only. It fails with OpenACC on Summit, but that's because some of our recent changes in array transfers weren't propagated to the si driver. So that's on us and I will open an issue on that and fix later.

Since I'm off on vacation for a bit, I will go ahead an approve based on my code review and the testing to date so I don't hold this up.

@hyungyukang
Copy link
Contributor Author

@philipwjones , Thanks for reviewing and approving this PR. Next PR will be GPU porting of the semi-implicit solver, so maybe I can look into that error.

Copy link
Contributor

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

I compiled and ran, with this feature off, with intel and gnu, debug and optimized and the nightly suite compares bfb with previous. Reviewed by visual inspection.

@hyungyukang
Copy link
Contributor Author

I compiled and ran, with this feature off, with intel and gnu, debug and optimized and the nightly suite compares bfb with previous. Reviewed by visual inspection.

@mark-petersen , Thanks Mark for testing and reviewing!

@jonbob jonbob added Stealth PR has feature which, if turned on, could change climate. fka FCC BFB PR leaves answers BFB labels Jun 30, 2022
jonbob added a commit that referenced this pull request Jun 30, 2022
Semi-implicit barotropic mode solver updates

This PR updates the semi-implicit barotropic mode solver:
* Restructure and make codes simpler in the main time-stepping code
* Make subroutines for the semi-implicit solver (preconditioining,
  matrix-vector multiplications, global reductions)
* Provide the s-step conjugate gradient method when
  config_btr_si_partition_match_mode is turned on to provide a faster
  symmetric iterative solver
* Small changes in stage 1 and 3 to match the split-explicit code

[BFB]
@jonbob
Copy link
Contributor

jonbob commented Jun 30, 2022

passes:

  • ERS.ne11_oQU240.WCYCL1850NS.chrysalis_intel
  • SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel
  • SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.chrysalis_intel
  • PEM_Ln9.ne30pg2_EC30to60E2r2.WCYCL1850.chrysalis_intel

merged to next

@jonbob jonbob merged commit 8380be0 into master Jul 1, 2022
@jonbob jonbob deleted the hkang/ocean/si-update branch July 1, 2022 21:08
@jonbob
Copy link
Contributor

jonbob commented Jul 1, 2022

merged to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB mpas-ocean Stealth PR has feature which, if turned on, could change climate. fka FCC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants