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

Possible bug in ocn_tracer_advection_std #841

Open
cbegeman opened this issue Apr 4, 2021 · 4 comments
Open

Possible bug in ocn_tracer_advection_std #841

cbegeman opened this issue Apr 4, 2021 · 4 comments

Comments

@cbegeman
Copy link

cbegeman commented Apr 4, 2021

I think that high_order_vert_flux should be allocated to size nVertLevels+1 here so that in the following lines, high_order_vert_flux(k+1) has a reference when maxLevelCell = nVertLevels. @mattdturner Do you agree?

do k = 1,maxLevelCell(iCell)
tend(iTracer, k, iCell) = tend(iTracer, k, iCell) + verticalDivergenceFactor(k) * (high_order_vert_flux(k+1, iCell) &
- high_order_vert_flux(k, iCell))
end do ! k loop

@mattdturner
Copy link
Collaborator

@cbegeman I agree that this is a bug as the code is currently written. It doesn't look like this particular subroutine is tested in the nightly regression suite (all tests have config_monotonic = .true.), so that would explain why it hasn't shown up as a FAIL before.

I'm not sure, though, if just allocating high_order_vert_flux to size nVertLevels+1 is the correct fix, or if the calculation of tend needs to be modified to handle k == maxLevelCell(iCell) a different way. If we increase the size of high_order_vert_flux by 1, then at the calculation of tend for k == maxLevelCell(iCell) high_order_vert_flux(k+1,iCell) will just have a value of 0.0 (which could very well be the desired behavior).

Perhaps @mark-petersen might have a better understanding of the calculations in this routine, and whether increasing the size of high_order_vert_flux to nVertLevels+1 is the correct resolution to this bug.

@cbegeman
Copy link
Author

cbegeman commented Apr 5, 2021

@mattdturner Thanks for taking a look. My understanding is that high_order_vert_flux(maxLevelCell(iCell)+1,iCell) corresponds to the tracer flux from the seafloor into the bottom cell. I do believe that for most applications this will be equal to 0 but perhaps for some tracers there would be entrainment (Fe, sediment). I'm not sure if that entrainment would be handled here or elsewhere.

@philipwjones
Copy link
Contributor

@mattdturner, @cbegeman Before I modified the mono advection routine, the vertical advection here was essentially identical, so I had planned to make the same performance mods in the std routine that I made in mono. I've been working on a version with GPU mods for both and will be making the changes as part of that, but if we want to make the changes sooner, I can speed that up or someone can copy how the vertical advection is done in the mono routine.

@cbegeman
Copy link
Author

cbegeman commented Apr 5, 2021

@philipwjones No rush on my end. Just wanted to point it out when I came across it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants