bug fix for m_opt: fixed units of surface momentum flux by matzegoebel · Pull Request #1214 · wrf-model/WRF

matzegoebel · 2020-06-19T13:55:24Z

TYPE: bug fix

KEYWORDS: m_opt, momentum flux

SOURCE: Matthias Göbel (University of Innsbruck)

DESCRIPTION OF CHANGES:
When outputting SGS momentum fluxes using the option m_opt, the units of the fluxes should be m2 s-2 according
to registry.les. The fluxes above the surface are divided by the density and thus have the correct units. The surface
fluxes, however, still contain the density.

This bug fix divides the surface fluxes by the density to obtain the correct units.

This change only affects the output of the SGS fluxes (nba_mij). The tendencies and thus the results are not changed.

LIST OF MODIFIED FILES:
dyn_em/module_diffusion_em.F

TESTS CONDUCTED:

Jenkins OK

RELEASE NOTE: Fixed units of surface momentum flux in dyn_em/module_diffusion_em.F when using m_opt. This bug fix divides the surface fluxes by the density to obtain the correct units. This change only affects the output of the SGS fluxes (nba_mij). The tendencies and thus the results are not changed.

TYPE: bug fix SOURCE: Matthias Göbel (UIBK) DESCRIPTION OF CHANGES: When outputting SGS momentum fluxes using the option m_opt, the units of the fluxes should be m2 s-2 according to the Registry. The fluxes above the surface are divided by the density and thus have the correct units. The surface fluxes, however, still contain the density. This bug fix divides the surface fluxes by the density to obtain the correct units. LIST OF MODIFIED FILES: dyn_em/module_diffusion_em.F TESTS CONDUCTED: 1. Jenkins regression test OK

davegill · 2020-06-19T15:03:52Z

@MATZE77 @weiwangncar @dudhia
When there are the introduction of new horizontal differences in algorithms, we must be concerned about boundary differences that would be caused by parallel communications. However, the d/dx and d/dy of rho already exists in those loops, so this is OK.

dudhia

Can we say that this is just an output change, not results?

davegill · 2020-07-10T21:00:53Z

@dudhia
Jimy,
So these are entirely diagnostic?

dudhia · 2020-07-10T21:30:46Z

That was my question. With some digging I could find out but it is easier to ask the proposer.

…

On Fri, Jul 10, 2020 at 3:01 PM Dave Gill ***@***.***> wrote: @dudhia <https://github.com/dudhia> Jimy, So these are entirely diagnostic? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1214 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEIZ77BKEIIL5N3JLG4PSZDR256RHANCNFSM4OCX7XBQ> .

dudhia · 2020-07-10T21:40:23Z

This code change needs to be run to check the impact anyway. It is an sfs_opt, m_opt combo.

…

On Fri, Jul 10, 2020 at 3:30 PM Jimy Dudhia ***@***.***> wrote: That was my question. With some digging I could find out but it is easier to ask the proposer. On Fri, Jul 10, 2020 at 3:01 PM Dave Gill ***@***.***> wrote: > @dudhia <https://github.com/dudhia> > Jimy, > So these are entirely diagnostic? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1214 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AEIZ77BKEIIL5N3JLG4PSZDR256RHANCNFSM4OCX7XBQ> > . >

matzegoebel · 2020-07-11T03:56:46Z

@dudhia @davegill
yes, this change only affects the output of the SGS fluxes (nba_mij). The tendencies and thus the results are not changed.

davegill · 2020-07-14T22:05:20Z

@dudhia @MATZE77
Jimy,
Are you OK with this PR now? Is there a test that you are requesting of Matthias?

dudhia · 2020-07-14T22:13:27Z

Either Matthias or our people could run a test to demonstrate the non-effect on other results when the change is made.

…

On Tue, Jul 14, 2020 at 4:05 PM Dave Gill ***@***.***> wrote: @dudhia <https://github.com/dudhia> @MATZE77 <https://github.com/Matze77> Jimy, Are you OK with this PR now? Is there a test that you are requesting of Matthias? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1214 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEIZ77GGY4TQX3TVXPFQFGDR3TJC5ANCNFSM4OCX7XBQ> .

matzegoebel · 2020-07-15T09:48:29Z

In my opinion, a test is not strictly necessary. I thoroughly checked that nba_mij, which is modified by this PR, is not used later on in any tendency calculations. The tendency calculations are happening right before the location of the changes. Thus, I don't want to invest time in a test run. If you want to have it tested anyway, you would have to do it yourself, I'm afraid. Or you ask the developers of that sfs_opt option. They can probably judge this PR much better than I can.

dudhia · 2020-07-15T15:13:45Z

Yes, we could do it internally. Thanks for this fix, Jimy

…

On Wed, Jul 15, 2020 at 3:48 AM Matthias Göbel ***@***.***> wrote: In my opinion, a test is not strictly necessary. I thoroughly checked that nba_mij, which is modified by this PR, is not used later on in any tendency calculations. The tendency calculations are happening right before the location of the changes. Thus, I don't want to invest time in a test run. If you want to have it tested anyway, you would have to do it yourself, I'm afraid. Or you ask the developers of that sfs_opt option. They can probably judge this PR much better than I can. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1214 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEIZ77A5KALN6WJZ6K2HBHTR3V3P3ANCNFSM4OCX7XBQ> .

weiwangncar · 2020-07-15T16:12:00Z

Should we contact the original contributor to check on this?

dudhia · 2020-07-15T17:19:02Z

He already replied in the thread that he won't test it but is sure of it not affecting results. Up to us to test it if we want.

…

On Wed, Jul 15, 2020 at 10:18 AM weiwangncar ***@***.***> wrote: Should we contact the original contributor to check on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1214 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEIZ77CLXGRRMAONN3YDW5LR3XJEBANCNFSM4OCX7XBQ> .

dudhia · 2020-07-15T17:19:57Z

I will look at the code and get back to you.

…

On Wed, Jul 15, 2020 at 11:19 AM Jimy Dudhia ***@***.***> wrote: He already replied in the thread that he won't test it but is sure of it not affecting results. Up to us to test it if we want. On Wed, Jul 15, 2020 at 10:18 AM weiwangncar ***@***.***> wrote: > Should we contact the original contributor to check on this? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1214 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AEIZ77CLXGRRMAONN3YDW5LR3XJEBANCNFSM4OCX7XBQ> > . >

dudhia

This only affects diagnostic output for m_opt=1.

weiwangncar · 2020-07-15T18:10:23Z

@dudhia I meant the original developer who contributed this option..

dudhia · 2020-07-15T18:13:10Z

It looks like a valid bug. I don't think we have any questions. If you want, that would be Jeff Mirocha.

…

On Wed, Jul 15, 2020 at 12:10 PM weiwangncar ***@***.***> wrote: @dudhia <https://github.com/dudhia> I meant the original developer who contributed this option.. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1214 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEIZ77BZPHDUYETVAN4UD4DR3XWKBANCNFSM4OCX7XBQ> .

weiwangncar · 2020-07-15T18:28:14Z

@dudhia I have sent an email to Jeff, and let's see if we can get any response.

@matzegoebel

TYPE: bug fix KEYWORDS: rho, sfs_opt, m_opt, cal_titau SOURCE: Robert Arthur and Jeff Mirocha (LLNL) DESCRIPTION OF CHANGES: Problem: In the cal_titau subroutines in module_diffusion_em, rho was not always interpolated to the correct location on the staggered grid. This issue affected the subgrid stress terms when sfs_opt .gt. 1. Additionally, when sfs_opt .eq. 0 and m_opt .eq. 1, the actual subgrid stresses applied in the code were correct, but the output was wrong. This is a follow-on from issue #1214 by @matzegoebel, which addressed the surface stress output from subroutine vertical_diffusion_2 when sfs_opt .gt. 0 or m_opt .eq. 1, but did not address the cal_titau subroutines. Solution: A rhoavg variable was added to cal_titau_12_21, cal_titau_13_31, and cal_titau_23_32 and used instead of the cell-centered rho variable. Additionally, because cal_titau_12_21 now uses "corner" ghost points for rho, a new halo/bc communication was added to solve_em.F to ensure that rho is specified correctly at those points. Correct specification of the corner points also requires the updates to subroutine set_physical_bc3d in #1314, included in the v4.2.2 release. Without the latter two updates for corner points, bit-for-bit errors occurred in regression tests for idealized, doubly-periodic domains. LIST OF MODIFIED FILES: dyn_em/module_diffusion_em.F dyn_em/solve_em.F Registry/Registry.EM_COMMON TESTS CONDUCTED: 1. Idealized, doubly-periodic tests with various processor breakdowns show bit-for-bit identical results. 2. Jenkins is all pass. RELEASE NOTE: Bug fix to use correct rho value in cal_titau subroutines. In the cal_titau subroutines in module_diffusion_em, rho was not always interpolated to the correct location on the staggered grid. This issue affected the subgrid stress terms when sfs_opt .gt. 1. Additionally, when sfs_opt .eq. 0 and m_opt .eq. 1, the actual subgrid stresses applied in the code were correct, but the output was wrong. A rhoavg variable was added to cal_titau_12_21, cal_titau_13_31, and cal_titau_23_32 and used instead of the cell-centered rho variable.

@matzegoebel

…1396) TYPE: bug fix KEYWORDS: rho, sfs_opt, m_opt, cal_titau SOURCE: Robert Arthur and Jeff Mirocha (LLNL) DESCRIPTION OF CHANGES: Problem: In the cal_titau subroutines in module_diffusion_em, rho was not always interpolated to the correct location on the staggered grid. This issue affected the subgrid stress terms when sfs_opt .gt. 1. Additionally, when sfs_opt .eq. 0 and m_opt .eq. 1, the actual subgrid stresses applied in the code were correct, but the output was wrong. This is a follow-on from issue wrf-model#1214 by @matzegoebel, which addressed the surface stress output from subroutine vertical_diffusion_2 when sfs_opt .gt. 0 or m_opt .eq. 1, but did not address the cal_titau subroutines. Solution: A rhoavg variable was added to cal_titau_12_21, cal_titau_13_31, and cal_titau_23_32 and used instead of the cell-centered rho variable. Additionally, because cal_titau_12_21 now uses "corner" ghost points for rho, a new halo/bc communication was added to solve_em.F to ensure that rho is specified correctly at those points. Correct specification of the corner points also requires the updates to subroutine set_physical_bc3d in wrf-model#1314, included in the v4.2.2 release. Without the latter two updates for corner points, bit-for-bit errors occurred in regression tests for idealized, doubly-periodic domains. LIST OF MODIFIED FILES: dyn_em/module_diffusion_em.F dyn_em/solve_em.F Registry/Registry.EM_COMMON TESTS CONDUCTED: 1. Idealized, doubly-periodic tests with various processor breakdowns show bit-for-bit identical results. 2. Jenkins is all pass. RELEASE NOTE: Bug fix to use correct rho value in cal_titau subroutines. In the cal_titau subroutines in module_diffusion_em, rho was not always interpolated to the correct location on the staggered grid. This issue affected the subgrid stress terms when sfs_opt .gt. 1. Additionally, when sfs_opt .eq. 0 and m_opt .eq. 1, the actual subgrid stresses applied in the code were correct, but the output was wrong. A rhoavg variable was added to cal_titau_12_21, cal_titau_13_31, and cal_titau_23_32 and used instead of the cell-centered rho variable.

davegill added bug Dynamics ARW release-v4.2.1 labels Jun 19, 2020

dudhia reviewed Jul 10, 2020

View reviewed changes

dudhia approved these changes Jul 15, 2020

View reviewed changes

kkeene44 merged commit 55da5ef into wrf-model:release-v4.2.1 Jul 15, 2020

rsarthur mentioned this pull request Dec 23, 2020

Bug fix to use correct rho value in cal_titau subroutines #1344

Closed

davegill mentioned this pull request Feb 9, 2021

Bug fix to use correct rho value in cal_titau subroutines #1396

Merged

Conversation

matzegoebel commented Jun 19, 2020 • edited by davegill Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davegill commented Jun 19, 2020

Uh oh!

dudhia left a comment

Choose a reason for hiding this comment

Uh oh!

davegill commented Jul 10, 2020

Uh oh!

dudhia commented Jul 10, 2020 via email

Uh oh!

dudhia commented Jul 10, 2020 via email

Uh oh!

matzegoebel commented Jul 11, 2020

Uh oh!

davegill commented Jul 14, 2020

Uh oh!

dudhia commented Jul 14, 2020 via email

Uh oh!

matzegoebel commented Jul 15, 2020

Uh oh!

dudhia commented Jul 15, 2020 via email

Uh oh!

weiwangncar commented Jul 15, 2020

Uh oh!

dudhia commented Jul 15, 2020 via email

Uh oh!

dudhia commented Jul 15, 2020 via email

Uh oh!

dudhia left a comment

Choose a reason for hiding this comment

Uh oh!

weiwangncar commented Jul 15, 2020

Uh oh!

dudhia commented Jul 15, 2020 via email

Uh oh!

weiwangncar commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

matzegoebel commented Jun 19, 2020 •

edited by davegill

Loading