Consistent wind metric - #791
thomasmelvin wants to merge 10 commits into
Conversation
|
Comparison results from weekly lfric_atm suite |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate transport-consistency and test-compatibility issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds opt-in consistent wind-metric corrections for Gungho transport, including reconstruction kernels, controller integration, tests, configuration, and Rose-stem coverage.
Changes:
- Adds cubic SL metric reconstruction and unit tests.
- Integrates consistent-wind processing into transport control.
- Adds configuration, tasks, and updated reference checksums.
Review findings:
- Critical (2 votes): FFSL is incorrectly treated as cubic SL; add FFSL-specific reconstruction or reject the combination.
- Critical (1 vote):
cheap_updatereinitialises the controller with uncorrected winds; reapply the correction or avoid replacing it. - Moderate (1 vote): A single reconstruction chosen from potential-temperature metadata is applied to fields with independently selected methods.
- Moderate (1 vote): Consistent-wind test arguments remain
r_tranafter the interface changed tor_def. - Moderate (1 vote): Metric-kernel tests pass
r_trantor_defarguments. - Moderate (1 vote): The metric kernel hard-codes Nirvana reconstruction despite FFSL supporting other orders.
- Nit (1 vote): Fix three typos:
suroutines,horzontal, andcomponenet. - Nit (2 votes): Add test coverage for the
dzdypath using non-zero meridional winds and varying profiles.
File summaries
| File | Description |
|---|---|
science/gungho/unit-test/kernel/transport/sl/horizontal_cubic_sl_metric_kernel_mod_test.pf |
Tests metric reconstruction. |
science/gungho/source/kernel/transport/sl/horizontal_cubic_sl_metric_kernel_mod.F90 |
Implements cubic SL metric gradients. |
science/gungho/source/kernel/transport/sl/horizontal_cubic_sl_kernel_mod.F90 |
Updates copyright year. |
science/gungho/source/kernel/transport/mol/consistent_wind_kernel_mod.F90 |
Updates consistent-wind precision and averaging. |
science/gungho/source/configuration/check_configuration_mod.F90 |
Removes spherical-geometry validation. |
science/gungho/source/algorithm/transport/mol/wt_advective_update_alg_mod.x90 |
Removes the legacy consistent-wind path. |
science/gungho/source/algorithm/transport/control/gungho_transport_control_alg_mod.X90 |
Invokes consistent-wind processing. |
science/gungho/source/algorithm/transport/control/consistent_wind_alg_mod.X90 |
Adds consistent-wind orchestration. |
rose-stem/site/meto/kgos/gungho_model/ex1a/checksum_gungho_model_straka_200m-alt2-BiP256x16-200x50_op_ex1a_gnu_fast-debug-64bit.txt |
Updates the reference checksum. |
rose-stem/site/meto/kgos/gungho_model/ex1a/checksum_gungho_model_schar_cart-alt2-BiP100x4-1000x1000_ex1a_gnu_fast-debug-64bit.txt |
Updates the reference checksum. |
rose-stem/site/meto/kgos/gungho_model/azspice/checksum_gungho_model_straka_200m-alt2-BiP256x16-200x50_op_azspice_gnu_fast-debug-64bit.txt |
Updates the reference checksum. |
rose-stem/site/meto/kgos/gungho_model/azspice/checksum_gungho_model_schar_cart-alt2-BiP100x4-1000x1000_azspice_gnu_fast-debug-64bit.txt |
Updates the reference checksum. |
rose-stem/site/meto/groups/groups_gungho_model.cylc |
Adds the consistent Schär test group. |
rose-stem/site/common/gungho_model/tasks_gungho_model.cylc |
Defines the consistent-metric task. |
rose-stem/app/gungho_model/opt/rose-app-consistent_metric.conf |
Enables the option. |
Review details
Suppressed comments (12)
science/gungho/source/algorithm/transport/control/consistent_wind_alg_mod.X90:160
- When
cheap_update_stepis true,transport_controller%initialisecomputes the transporting wind as0.5*(wind_np1-wind_n)(seetransport_controller_mod.x90:269-273). Passingwind_advas both arguments here therefore makes the transporting wind exactly zero, so every cheap-update outer loop performs no transport. Preserve distinct corrected n/np1 winds, or initialise with a directly corrected transporting wind instead of reusing one field for both.
call transport_controller%finalise()
call transport_controller%initialise( &
config, model_clock, rho_d_latest, wind_adv, wind_adv_ptr, outer, &
cheap_update_step &
science/gungho/source/algorithm/transport/control/consistent_wind_alg_mod.X90:142
- For a spherical cubed-sphere mesh, the normal SL path uses
horizontal_cubic_sl_sphere_kernel_typewith panel and edge handling (horizontal_sl_advective_alg.x90:273-285), but this path always uses the planar CROSS2D metric kernel. Removing the former spherical-geometry validation now allowsconsistent_metric=.true.to compute a different, invalid horizontal metric at panel edges. Keep the restriction or provide a sphere-aware metric kernel.
! With SL
call invoke(horizontal_cubic_sl_metric_kernel_type(chi3_inc, chi3, stencil_depth, wind_h) )
end if
science/gungho/source/algorithm/transport/control/consistent_wind_alg_mod.X90:107
- This chooses the reconstruction solely from the
potential_temperaturemetadata, but the rebuilt controller is then used for density, wind, moisture, and tracer transports, whose metadata is selected independently and may specify different horizontal methods. In those valid per-variable configurations, the single correction cannot match every transport reconstruction. Either validate that all affected fields use a compatible method when this option is enabled, or compute method-specific corrections/controllers.
! Transport the height field
call log_event( "Computing consistent vertical wind...", LOG_LEVEL_INFO)
transport_metadata => &
transport_metadata_collection%get_transport_metadata('potential_temperature')
science/gungho/source/algorithm/transport/control/consistent_wind_alg_mod.X90:37
- Correct the typo in this new comment:
suroutinesshould besubroutines.
! Contained suroutines
science/gungho/source/algorithm/transport/control/consistent_wind_alg_mod.X90:110
- Correct the typo in this new comment:
horzontalshould behorizontal.
! Split out the horzontal and vertical components of the wind
science/gungho/source/algorithm/transport/control/consistent_wind_alg_mod.X90:144
- Correct the typo in this new comment:
componenetshould becomponent.
! Modify the vertical componenet of the transporting wind
science/gungho/source/algorithm/transport/control/gungho_transport_control_alg_mod.X90:336
- The old configuration check prevented
consistent_metricon spherical geometry, but this unconditional call now enables it without a spherical metric path. The invoked kernel only has a CROSS2D stencil and no panel ID/edge-distance or component-rotation inputs, whereas the normal spherical transport dispatches to panel-edge-aware kernels; cells crossing cubed-sphere panel edges will therefore use the wrong reconstructed wind. Restore the restriction or dispatch to a spherical metric implementation.
if (consistent_metric) call consistent_wind_alg(config, &
transport_controller, &
rho_d_latest_ptr, &
wind_for_adv, &
wind_np1, &
model_clock, &
outer, &
cheap_update_step)
science/gungho/source/kernel/transport/mol/consistent_wind_kernel_mod.F90:119
- Changing these field arguments from
r_trantor_defleavesconsistent_wind_kernel_mod_test.pfpassingreal(r_tran)arrays. That test deliberately handles both 32- and 64-bit transport precision; whenr_trandiffers fromr_def, the explicit module-procedure call is kind-mismatched and the unit-test build fails. Update the test arguments tor_def(or keep the kernel interface consistent with its callers).
real(kind=r_def), dimension(undf_w2v), intent(inout) :: consistent_wind
real(kind=r_def), dimension(undf_wt), intent(in) :: theta_metrics
real(kind=r_def), dimension(undf_w2), intent(in) :: wind
real(kind=r_def), dimension(undf_wx), intent(in) :: height
science/gungho/source/kernel/transport/sl/horizontal_cubic_sl_metric_kernel_mod.F90:102
- The new kernel interface requires
real(kind=r_def), but its unit test passesreal(kind=r_tran)arrays, and the existingconsistent_wind_kernel_mod_test.pfstill does the same after that kernel was changed tor_def. The tests explicitly supportr_trandiffering fromreal64; with a differing transport precision, these explicit module-procedure calls are kind-mismatched and will not compile. User_defin the field arguments of these tests (or provide a matching interface).
real(kind=r_def), intent(inout) :: increment(undf_wf)
real(kind=r_def), intent(in) :: z(undf_wf)
real(kind=r_def), intent(in) :: wind(undf_w2h)
science/gungho/source/kernel/transport/sl/horizontal_cubic_sl_metric_kernel_mod.F90:116
- The metric reconstruction is hard-coded to the Nirvana coefficients here, while FFSL selects constant, Nirvana, or PPM reconstruction from its configured order (
ffsl_flux_xy_kernel_mod.F90:545-578, with orders 0, 1, and 2). Withconsistent_metricenabled and an FFSL order other than Nirvana, the vertical-wind correction therefore does not use the same reconstruction as the horizontal transport. Pass the configured reconstruction/order to this kernel or reject incompatible configurations.
! Interpolation coefficients
real(kind=r_def), parameter :: b0 = -1.0_r_def/6.0_r_def
real(kind=r_def), parameter :: b1 = 5.0_r_def/6.0_r_def
real(kind=r_def), parameter :: b2 = 2.0_r_def/6.0_r_def
real(kind=r_def), parameter :: c1 = 2.0_r_def/6.0_r_def
real(kind=r_def), parameter :: c2 = 5.0_r_def/6.0_r_def
real(kind=r_def), parameter :: c3 = -1.0_r_def/6.0_r_def
science/gungho/source/kernel/transport/sl/horizontal_cubic_sl_metric_kernel_mod.F90:10
- The reconstruction name is misspelled as
Nivanain both new documentation lines; use the established scheme nameNirvana.
!> @brief Compute advective increment u.grad(z) using the Nivana scheme used in
!! the ffsl and sl transport schemes.
!> @details Compute the advective increment u.grad(z) = u*dz/dx + v*dz/dy where
!! the horizontal gradients are computed using the same reconstruction method as
!! the transport scheme (in this case Nivana).
science/gungho/unit-test/kernel/transport/sl/horizontal_cubic_sl_metric_kernel_mod_test.pf:10
- The production metric kernel declares its field arguments as
real(kind=r_def), but this test allocates the fields asr_tran. That is an explicit-interface kind mismatch when transport precision differs from default precision (and the same mismatch remains in the existing consistent-wind kernel test after its interface change), so the non-64-bit test configuration cannot reliably compile or exercise this kernel. Use the same kind as the kernel for the test fields and update the tolerance logic accordingly.
use constants_mod, only : i_def, r_tran
- Files reviewed: 15/15 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Your CLA signature was found on the base branch, but you appear to have modified the CONTRIBUTORS.md file in this PR. Please do not edit the CONTRIBUTORS.md file. If you have already signed the CLA, revert changes to the file and your signature will be picked up. |
PR Summary
Sci/Tech Reviewer: Thomas Bendall (@tommbendall)
Code Reviewer: Matthew Walker (@mattatmet)
The application of consistent wind metric terms is detailed in Section 5.3.3 of the Cartesian Gungho paper: https://rmets.onlinelibrary.wiley.com/doi/10.1002/qj.3501 here the methodology is extended to general coordinate systems and transport schemes.
It has been shown (see references in https://rmets.onlinelibrary.wiley.com/doi/10.1002/qj.3501) that with terrain following coordinates an inconsistency can develop between the orographic gradient (dz/dx like terms) used to compute the wind along terrain following coordinates (which in lfric will use the finite element basis polynomials) and the dz/dx term in transporting a strongly height dependent term (which will use whatever reconstruction term is used in the transport scheme). This inconsistency manifests as a distortion in the orographic waves produced over orography. The solution is it modify the vertical wind term that is used to transport fields so that the dz/dx term that comes from the finite element basis polynomials is replaced by one computed using the reconstruction used in the transport scheme.
Currently this code is only tested in a few gungho tests. I have included some comparison lfric_atm tests with this turned on for reference but it is currently off by default in this PR pending further, fuller, assesment in case studies.
Code Quality Checklist
Testing
trac.log
thomas.melvin@cazldf0000D5:]:>cat ../results/consistent_metrics/trac.log
Test Suite Results - lfric_apps - consistent_wind_metric/run18
Suite Information
Task Information
✅ succeeded tasks - 1213
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review