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

remove unused variables in ocean #5610

Merged
merged 1 commit into from
Apr 28, 2023
Merged

remove unused variables in ocean #5610

merged 1 commit into from
Apr 28, 2023

Conversation

mark-petersen
Copy link
Contributor

@mark-petersen mark-petersen commented Apr 19, 2023 •
edited
Loading

The variables tidalPotentialZMid and nTidalPotentialConstituents are defined in the Registry but not used for any computations. This tricks the gnu compiler in optimized mode, which removes them internally. The simulation then dies when the i/o references these variables on initialization. See earlier description at MPAS-Dev/compass#497.

With this fix, the standalone MPAS-Ocean tests pass

ocean_global_ocean_EC30to60_PHC_performance_test
ocean_global_ocean_ECwISC30to60_PHC_performance_test

fixes #5609
[BFB]

mark-petersen
mark-petersen commented Apr 19, 2023
View reviewed changes
components/mpas-ocean/src/shared/mpas_ocn_vel_tidal_potential.F
@@ -434,9 +430,6 @@ subroutine ocn_vel_tidal_potential_init(domain,err)!{{{
constituentList(nTidalConstituents)%constituent = 'P1'
end if

! set point value in forcing pool just in case
nTidalPotentialConstituents = nTidalConstituents
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbrus89 please check if I can remove nTidalPotentialConstituents. It looks like you added it to record information during development. This would never change, so I suspect you don't need it anymore. The gnu compiler seems to think you don't need it either.

components/mpas-ocean/src/Registry.xml
@@ -3792,10 +3788,6 @@
description="Latitude function for tidal constituents: long-period = 3\sin^2(\phi)-1, diurnal = \sin(2\phi), semi-diurnal = \cos^2(\phi)"
packages="tidalPotentialForcingPKG"
/>
<var name="tidalPotentialZMid" type="real" dimensions="nVertLevels nCells Time" units="m"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbrus89 please verify that I can remove tidalPotentialZMid. It appears to be a remnant of something during development.

@mark-petersen
Copy link
Contributor Author

Just a note: These two variables were not actually listed in the streams.ocean file, making the problem harder to diagnose. They came in with

<stream name="forcing_data"
        type="input"
        filename_template="forcing_data.nc"
        input_interval="initial_only">
...
<var_struct name="forcing"/>

along with 60 other variables in the forcing structure!

@xylar
Copy link
Contributor

xylar commented Apr 19, 2023

@mark-petersen, wow, thank you for figuring this out!

xylar
xylar approved these changes Apr 19, 2023
View reviewed changes
Copy link
Contributor

@xylar xylar 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.

Approved with great enthusiasm! This has fixed the PR test suite on Perlmutter and Chicoma!

@xylar xylar mentioned this pull request Apr 19, 2023
Closed
@mark-petersen mark-petersen mentioned this pull request Apr 20, 2023
Closed
@mark-petersen
Copy link
Contributor Author

mark-petersen commented Apr 20, 2023 •
edited
Loading

@mark-petersen, wow, thank you for figuring this out!

Thanks! It's funny - with all our advanced training, debugging still comes down to a few hours of trial and error. In this case I had to run the case with all the variables in the gigantic forcing var_struct individually listed in the stream, and then parse them down until I caught the troublemakers!

sbrus89
sbrus89 approved these changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@sbrus89 sbrus89 left a comment

Choose a reason for hiding this comment

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

@mark-petersen, yes both of these are unused and are development remnants. Thanks for catching this. Sorry for the trouble!

jonbob added a commit that referenced this pull request Apr 27, 2023
@jonbob
Merge branch 'mark-petersen/ocn/remove-unused-variables' into next (PR …
80ef1e6 
…#5610)

Remove unused variables in ocean

The variables tidalPotentialZMid and nTidalPotentialConstituents are
defined in the Registry but not used for any computations. This tricks
the gnu compiler in optimized mode, which removes them internally. The
simulation then dies when the i/o references these variables on
initialization.

Fixes #5609
[BFB]
@jonbob
Copy link
Contributor

jonbob commented Apr 27, 2023

passes:

  • SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel
  • SMS_D_Ld1.ne30pg2_EC30to60E2r2.WCYCL1850.chrysalis_intel.allactive-wcprod
  • ERS.ne11_oQU240.WCYCL1850NS.chrysalis_intel

merged to next

@jonbob jonbob merged commit de075c7 into E3SM-Project:master Apr 28, 2023
@jonbob
Copy link
Contributor

jonbob commented Apr 28, 2023

merged to master

@xylar xylar mentioned this pull request Apr 29, 2023
Merged
3 tasks
@mark-petersen mark-petersen deleted the mark-petersen/ocn/remove-unused-variables branch May 8, 2023 02:39
@darincomeau darincomeau mentioned this pull request Jun 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbrus89 sbrus89 sbrus89 approved these changes

@xylar xylar xylar approved these changes

Assignees

@jonbob jonbob

Labels
bug fix PR mpas-ocean
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ocean EC(wISC)30to60 performance tests fail
4 participants
@mark-petersen @xylar @jonbob @sbrus89