Skip to content

Adds option to homogenize forces and fluxes fields#1541

Closed
breichl wants to merge 4 commits into
mom-ocean:dev/gfdlfrom
breichl:user/bgr/Homogenize_forces_and_fluxes
Closed

Adds option to homogenize forces and fluxes fields#1541
breichl wants to merge 4 commits into
mom-ocean:dev/gfdlfrom
breichl:user/bgr/Homogenize_forces_and_fluxes

Conversation

@breichl
Copy link
Copy Markdown
Collaborator

@breichl breichl commented Nov 8, 2021

  • Adds functions to do global averages on U and V grids in MOM_spatial_means
  • Adds functionality to average all forcing and fluxes fields in MOM_forcing_types
  • Adds flag to average all forcing and fluxes in MOM.F90
  • This new feature is primarily for running in single column like configurations with the coupler, which requires perfectly equal forcing across all cells.

- Adds functions to do global averages on U and V grids in MOM_spatial_means
- Adds functionality to average all forcing and fluxes fields in MOM_forcing_types
- Adds flag to average all forcing and fluxes in MOM.F90
- This new feature is primarily for running in single column like configurations with the coupler, which requires perfectly equal forcing across all cells.
@breichl
Copy link
Copy Markdown
Collaborator Author

breichl commented Nov 8, 2021

This code is meant to replace #1480 @Hallberg-NOAA let me know what you think of these changes. I still have testing and some functionality to complete, but I wanted to get your opinion on these changes compared to the original PR.

One thing I'm unsure of is the best way to average ustar in the fluxes routine. In forces it is fine, since we have taux and tauy. The options I can think of so far to get the correct ustar in fluxes are (1) to recompute the fluxes ustar from forces by passing forces to the fluxes homogenization routine, (2) to add the stress information (perhaps at h-points) to fluxes structure. I also need to either add functionality to read the gustiness or add the gustiness factor into the forces control structure.

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 8, 2021

Codecov Report

Merging #1541 (533dccc) into dev/gfdl (1324656) will decrease coverage by 0.06%.
The diff coverage is 1.81%.

❗ Current head 533dccc differs from pull request most recent head 7e3f66a. Consider uploading reports for the commit 7e3f66a to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##           dev/gfdl    #1541      +/-   ##
============================================
- Coverage     29.15%   29.09%   -0.07%     
============================================
  Files           239      239              
  Lines         71533    71698     +165     
============================================
+ Hits          20855    20858       +3     
- Misses        50678    50840     +162     
Impacted Files Coverage Δ
src/core/MOM_forcing_type.F90 43.04% <0.00%> (-4.18%) ⬇️
src/diagnostics/MOM_spatial_means.F90 11.87% <0.00%> (-1.60%) ⬇️
src/core/MOM.F90 58.89% <37.50%> (-0.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1324656...7e3f66a. Read the comment docs.

@breichl breichl requested a review from Hallberg-NOAA November 8, 2021 22:06
@breichl breichl mentioned this pull request Nov 8, 2021
- Adds in irho0 and sqrt that were missing in homogenize mech forcing.
@Hallberg-NOAA
Copy link
Copy Markdown
Collaborator

I think that this PR is a great improvement over its predecessor, and I like the general direction it is going. There are a few comments or issues that I see with this.

  1. The new routines global_area_mean_u() and global_area_mean_v() will not reproduce across PE layouts when used in symmetric memory mode, and they will not reproduce across rotation by 180 degrees when used in non-symmetric memory mode. The basic problem is that they will double-count the velocity points at the edges of the PEs, or only count the ones on the one edge. In addition, it is using the integrated area of the unmasked tracer points, not the velocity points. For these routines, I think that the solution may be to do an appropriate interpolation of the velocities or stresses to the tracer points (perhaps with land masking and area-weighted averaging), and then do the global average on the tracer points.

  2. What is wrong with taking the area-weighted average of ustar at the tracer points directly? Yes, it will be larger than the ustar that would be based on the area-averaged stresses, but in the same way that the sub-gridscale gustiness is something that we regularly include when calculating ustar in the first place, this difference is the legitimate result of the averaging operator not commuting with the nonlinear conversion of the velocities into ustar. As long as we are explicit about how the averaged ustar is calculated, why do we care if it can not be directly derived from the averaged stresses? If we do care about ustar being exactly the average of taux and tauy, that would probably be handled best in a separate subroutine to copy the information from a mech_forcing type to a forcing type, with the call controlled by another run-time flag to determine whether to calculate ustar that way.

  3. There is a lot of repetition of nearly identical blocks of about 6 lines to take the averages in the new homogenize_forcing() and homogenize_mech_forcing() routines. (There are 53 such blocks for tracer points by my count.) The code might be a lot shorter (by perhaps 300 lines) and less error prone if a small local subroutine were introduced to perform this task of replacing a field with its average in a single-line call.

- Correct issues in global_area_mean_u and global_area_mean_v to work with symmetric and rotated grids.
- Add options to compute mean ustar or compute ustar from mean tau.
- Add subroutines to replace averaging blocks in MOM_forcing_type.
Comment thread src/core/MOM_forcing_type.F90 Outdated
Copy link
Copy Markdown
Collaborator Author

@breichl breichl left a comment

Choose a reason for hiding this comment

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

I caught a couple formatting issues here and wanted to raise two other issues here.

if (do_iceberg) then
call rotate_array(fluxes_in%ustar_berg, turns, fluxes%ustar_berg)
call rotate_array(fluxes_in%area_berg, turns, fluxes%area_berg)
!BGR: pretty sure the following line isn't supposed to be here.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I noticed this is a repeat of code above. I think it is unintentional.

Comment thread src/core/MOM_forcing_type.F90 Outdated
if (associated(fluxes%ustar_tidal)) &
call homogenize_field_t(fluxes%ustar_tidal, G)

! TODO: tracer flux homogenization
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't know if this is worth addressing before taking this PR?

Comment thread src/diagnostics/MOM_spatial_means.F90 Outdated

tmpForSumming(:,:) = 0.
do J=js,je ; do i=is,ie
tmpForSumming(i,j) = G%areaT(i,j)/max(1.e-20,&
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fix formatting.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In this expression, I think that we would prefer to have division at the end, or else put parentheses around the area divided by the masks, so that someone reading the code does not need to remember the order of precedence in Fortran between multiplication and division (they are the same), or whether the Fortran order of operations goes left to right or right to left. The same comment would apply in global_area_mean_u().

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The most recent code push should address this.

@breichl
Copy link
Copy Markdown
Collaborator Author

breichl commented Nov 9, 2021

I think that this PR is a great improvement over its predecessor, and I like the general direction it is going. There are a few comments or issues that I see with this.

1. The new routines `global_area_mean_u()` and `global_area_mean_v()` will not reproduce across PE layouts when used in  symmetric memory mode, and they will not reproduce across rotation by 180 degrees when used in non-symmetric memory mode.  The basic problem is that they will double-count the velocity points at the edges of the PEs, or only count the ones on the one edge.  In addition, it is using the integrated area of the unmasked tracer points, not the velocity points. For these routines, I think that the solution may be to do an appropriate interpolation of the velocities or stresses to the tracer points (perhaps with land masking and area-weighted averaging), and then do the global average on the tracer points.

2. What is wrong with taking the area-weighted average of ustar at the tracer points directly?  Yes, it will be larger than the ustar that would be based on the area-averaged stresses, but in the same way that the sub-gridscale gustiness is something that we regularly include when calculating ustar in the first place, this difference is the legitimate result of the averaging operator not commuting with the nonlinear conversion of the velocities into ustar.  As long as we are explicit about how the averaged ustar is calculated, why do we care if it can not be directly derived from the averaged stresses?  If we do care about ustar being exactly the average of taux and tauy, that would probably be handled best in a separate subroutine to copy the information from a `mech_forcing` type to a `forcing` type, with the call controlled by another run-time flag to determine whether to calculate ustar that way.

3. There is a lot of repetition of nearly identical blocks of about 6 lines to take the averages in the new `homogenize_forcing()` and `homogenize_mech_forcing()` routines.  (There are 53 such blocks for tracer points by my count.)  The code might be a lot shorter (by perhaps 300 lines) and less error prone if a small local subroutine were introduced to perform this task of replacing a field with its average in a single-line call.

Thanks for the review and suggestions, @Hallberg-NOAA.

I think I've fixed (1) based on our discussion offline. It now interpolates u/v point variables to tracer points and does the averaging there. I don't have a test for this yet.

I've added options for (2) to either compute ustar from the mean tau or as the mean of ustar. We cannot include gustiness at this point; it could be added but would require either a new get_param or passing it to the forces module via each cap. Since it is not clear how to do it and not clear that we need it I've put that decision off for now.

For (3) I've added three new subroutines to do the averaging now, which indeed reduces the file lengths.

- Move a division and reformat alignment in MOM_spatial_means.F90.
- Remove a unused parameter in MOM_forcing_type.F90
- Reformat misalignment of an "if-block" in MOM_forcing_type.F90
@breichl
Copy link
Copy Markdown
Collaborator Author

breichl commented Nov 10, 2021

@nikizadehgfdl can you try this at some point in your workflow? It is a little different than the previous attempt, but I believe will have the same functionality. The replacement parameter to activate homogenized forcing with this code will be "HOMOGENIZE_FORCINGS = True"

@adcroft adcroft deleted the branch mom-ocean:dev/gfdl November 16, 2021 20:03
@adcroft adcroft closed this Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants