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

Fixes shortwave absorption for shallow cells #804

Merged

Conversation

vanroekel
Copy link
Contributor

Currently for very shallow cells some of the incident shortwave
radiation is not applied to the column. This adds that missing heating
back to the bottom cell.

Currently for very shallow cells some of the incident shortwave
radiation is not applied to the column.  This adds that missing heating
back to the bottom cell.
@vanroekel vanroekel marked this pull request as draft February 5, 2021 05:13
@vanroekel vanroekel marked this pull request as ready for review February 12, 2021 14:18
@vanroekel
Copy link
Contributor Author

@maltrud could you post a quick figure here showing this fixes the non conservation of heat?

@mark-petersen this PR is needed as soon as we can. The ocean has a non-conservation of heat right now, it is small but definitely there. If we could make this PR high priority I'd appreciate it.

@maltrud
Copy link
Contributor

maltrud commented Feb 15, 2021

i think the OMP loop needs to have fluxRemaining as a private variable?

   !$omp do schedule(runtime) private(depth, k, depLev) &

should be
!$omp do schedule(runtime) private(fluxRemaining, depth, k, depLev) &

@maltrud
Copy link
Contributor

maltrud commented Feb 15, 2021

in my version, i did the bottom check the same way as the other heat fluxes. i'm not sure it matters, but i thought i'd point it out:
Luke's:
< k = maxLevelCell(iCell)
< tend(index_temperature, k, iCell) = tend(index_temperature, k, iCell) + penetrativeTemperatureFlux(iCell) &
< * fluxRemaining

Mat's

     if(maxLevelCell(iCell) > 0 .and. fluxRemaining > 0.0_RKIND) then
       tend(index_temperature, maxLevelCell(iCell), iCell) = tend(index_temperature, maxLevelCell(iCell), iCell) &
                                                 + penetrativeTemperatureFlux(iCell) * fluxRemaining
     end if

@maltrud
Copy link
Contributor

maltrud commented Feb 16, 2021

here is a little blurb that demonstrates the scale of the problem and the improvement with this fix:
Screen Shot 2021-02-15 at 5 02 19 PM

@vanroekel
Copy link
Contributor Author

@maltrud thanks for catching the missing thread private variable. I will fix that.

On the other form for fluxRemaining. Do we have places where maxLevelCell(iCell) = 0? I don't think that is allowed. The other check on fluxRemaining > 0 also seems unnecessary to me, for the exponential it should never be less than 0 and if it is zero it seems fine to just do the calculation, perhaps more performant? @mark-petersen what do you think? I can add it back if you'd like

@xylar
Copy link
Collaborator

xylar commented Feb 16, 2021

Do we have places where maxLevelCell(iCell) = 0? I don't think that is allowed.

Our current meshes cull out any such cells but MPAS-Ocean is supposed to run in situations where maxLevelCell < 1 is used to indicate an inactive ocean column. This could become very important for us with grounding line motion, so please do try to check for that. I'm guessing we should add a test case to COMPASS with an unculled mesh (say, QU240) to make sure this works.

@vanroekel
Copy link
Contributor Author

Great point @xylar I will add that check back in.

@vanroekel
Copy link
Contributor Author

I just added a check on maxLevelCell as in the heat flux case and what @maltrud posts here. @mark-petersen could you please take a look as well? I think this is ready to go.

mark-petersen added a commit that referenced this pull request Feb 17, 2021
Fixes shortwave absorption for shallow cells #804
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.

This looks good. Tested with gnu and intel, both debug, on nightly regression suite.

jonbob added a commit to E3SM-Project/E3SM that referenced this pull request Feb 17, 2021
Update mpas-ocean: Fixes shortwave absorption for shallow cells

Brings in a new mpas-source submodule with changes only to the ocean
core. It fixes an issue where, for very shallow cells, some of the
incident shortwave radiation is not applied to the column. This adds that
missing heating back to the bottom cell.

see MPAS-Dev/MPAS-Model#804

[non-BFB]
jonbob added a commit to E3SM-Project/E3SM that referenced this pull request Feb 18, 2021
Update mpas-ocean: Fixes shortwave absorption for shallow cells

Brings in a new mpas-source submodule with changes only to the ocean
core. It fixes an issue where, for very shallow cells, some of the
incident shortwave radiation is not applied to the column. This adds that
missing heating back to the bottom cell.

see MPAS-Dev/MPAS-Model#804

[non-BFB]
@mark-petersen mark-petersen merged commit 2d69f6a into MPAS-Dev:ocean/develop Feb 18, 2021
mark-petersen added a commit to mark-petersen/MPAS-Model that referenced this pull request Mar 16, 2021
…evelop

Fixes shortwave absorption for shallow cells MPAS-Dev#804

Currently for very shallow cells some of the incident shortwave
radiation is not applied to the column. This adds that missing heating
back to the bottom cell.
mark-petersen added a commit to mark-petersen/MPAS-Model that referenced this pull request Mar 16, 2021
Fixes shortwave absorption for shallow cells MPAS-Dev#804
vanroekel pushed a commit to vanroekel/MPAS-Model that referenced this pull request Sep 20, 2021
…evelop

Fixes shortwave absorption for shallow cells MPAS-Dev#804

Currently for very shallow cells some of the incident shortwave
radiation is not applied to the column. This adds that missing heating
back to the bottom cell.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants