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

Add missing omp private variables for data subglacial runoff #6656

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

jonbob
Copy link
Contributor

@jonbob jonbob commented Oct 1, 2024

Adds some variables were introduced in PR #6508 and not icluded in the appropriate omp private statement. These variables are only used for data subglacial runoff, so this does not impact current tests.

[BFB]

@jonbob jonbob added mpas-ocean BFB PR leaves answers BFB labels Oct 1, 2024
@jonbob jonbob requested a review from vanroekel October 1, 2024 20:33
@jonbob jonbob self-assigned this Oct 1, 2024
Copy link

github-actions bot commented Oct 1, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6656/
on branch gh-pages at 2024-10-03 19:41 UTC

@jonbob
Copy link
Contributor Author

jonbob commented Oct 1, 2024

@vanroekel -- did you see any other loops where the new variables didn't get included?

@jonbob
Copy link
Contributor Author

jonbob commented Oct 1, 2024

I ran the following test with and without these changes:

  • PET_P640.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF-DSGR.chrysalis_intel.mpaso-jra_1958
    I didn't see a FAIL in any testing, but it is still worth making these fixes

@rljacob
Copy link
Member

rljacob commented Oct 1, 2024

You may want to add that test to e3sm_gpuomp

Copy link
Contributor

@vanroekel vanroekel left a comment

Choose a reason for hiding this comment

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

Approving by visual inspection. This is the only place I saw some omp variables missing. Thanks @jonbob

Copy link
Contributor

@irenavankova irenavankova left a comment

Choose a reason for hiding this comment

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

Approving based on visual inspection. Thank you for fixing this!

@irenavankova
Copy link
Contributor

During the check I spotted another typo, on lines 3576-3577, there should be config_flux_attenuation_coefficient_subglacial_runoff instead of config_flux_attenuation_coefficient_runoff as follows:

transmissionCoeffTop = exp( max(zTop / config_flux_attenuation_coefficient_subglacial_runoff, -100.0_RKIND) )
transmissionCoeffBot = exp( max(zBot / config_flux_attenuation_coefficient_subglacial_runoff, -100.0_RKIND) )

@jonbob, could that be included in this pull request or do I need to open a new one?

@jonbob
Copy link
Contributor Author

jonbob commented Oct 2, 2024

@irenavankova -- that should probably be a new PR. Do you want to make it?

@irenavankova
Copy link
Contributor

Yes, I ll make it.

@jonbob
Copy link
Contributor Author

jonbob commented Oct 3, 2024

@rljacob -- we could also change the test from the PR that brought this in. Right now we include

SMS_P480_Ld5.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF-DSGR.mpaso-jra_1958

in the e3sm_ocnice_extra_coverage suite. We could change that to

PET_P480_Ld2.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF-DSGR.mpaso-jra_1958

@rljacob
Copy link
Member

rljacob commented Oct 3, 2024

Sure. And I realized this isn't omp-offload you're talking about so you don't need to move it to gpuomp

jonbob added a commit that referenced this pull request Oct 3, 2024
Adds some variables were introduced in PR #6508 and not icluded in the
appropriate omp private statement. These variables are only used for
data subglacial runoff, so this does not impact current tests. Also
changes the current GMPAS-JRA1p5-DIB-PISMF-DSGR test to PET to watch
for threading issues.

[BFB]
@jonbob
Copy link
Contributor Author

jonbob commented Oct 3, 2024

Passes:

  • PET_P480.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF-DSGR.chrysalis_intel.mpaso-jra_1958 (new test)
  • ERP_Ld3.ne30pg2_r05_IcoswISC30E3r5.WCYCL1850.chrysalis_intel.allactive-pioroot1

merged to next

@jonbob jonbob merged commit 169448a into master Oct 4, 2024
9 checks passed
@jonbob jonbob deleted the jonbob/mpaso/fix-subglacial-omp branch October 4, 2024 15:50
@jonbob
Copy link
Contributor Author

jonbob commented Oct 4, 2024

merged to master

xylar added a commit to xylar/compass that referenced this pull request Oct 26, 2024
This merge updates the E3SM-Project submodule from [727ad81](https://github.com/E3SM-Project/E3SM/tree/727ad81) to [1442143](https://github.com/E3SM-Project/E3SM/tree/1442143).

This update includes the following MPAS-Ocean and MPAS-Frameworks PRs (check mark indicates bit-for-bit with previous PR in the list):
- [ ]  (ocn) E3SM-Project/E3SM#6509
- [ ]  (ocn) E3SM-Project/E3SM#6508
- [ ]  (fwk) E3SM-Project/E3SM#6575
- [ ]  (ocn) E3SM-Project/E3SM#6590
- [ ]  (fwk) E3SM-Project/E3SM#6643
- [ ]  (ocn) E3SM-Project/E3SM#6656
- [ ]  (ocn) E3SM-Project/E3SM#6672
- [ ]  (ocn) E3SM-Project/E3SM#6659
- [ ]  (ocn) E3SM-Project/E3SM#6497
- [ ]  (ocn) E3SM-Project/E3SM#6485
- [ ]  (ocn) E3SM-Project/E3SM#6566
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants