Skip to content

Remove base mesh dependency in compute sample kernel #313

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.

Sign up for GitHub

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

Open
Juan Manuel Castillo Sanchez (ukmo-juan-castillo) wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Remove base mesh dependency in compute sample kernel #313

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4aff176
Obtain geometry and topology information for mapping from the mesh
ukmo-juan-castillo Mar 24, 2026
c21acf9
Modify test not to use 'magic' variables
ukmo-juan-castillo Mar 24, 2026
6a2a993
Fix a typo
ukmo-juan-castillo Mar 24, 2026
dc23584
Add name to contributors list
ukmo-juan-castillo Mar 24, 2026
d7eb78c
Pass the integer values of geometry and topology to the kernel instead
ukmo-juan-castillo Mar 25, 2026
a5fa24a
Minor fixes
ukmo-juan-castillo Mar 25, 2026
93e07d4
Some more small fixes
ukmo-juan-castillo Mar 26, 2026
3a107d4
Merge branch 'main' into base_mesh_compute_sample_kernel
ukmo-juan-castillo Mar 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 26 additions & 24 deletions CONTRIBUTORS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,28 @@
# Contributors

| GitHub user | Real Name | Affiliation | Date |
| ---------------- | ---------------------- | ----------- | ---------- |
| andrewcoughtrie | Andrew Coughtrie | Met Office | 2025.12.12 |
| james-bruten-mo | James Bruten | Met Office | 2025-12-09 |
| jedbakerMO | Jed Baker | Met Office | 2025-12-29 |
| jennyhickson | Jenny Hickson | Met Office | 2025-12-10 |
| mo-marqh | Mark Hedley | Met Office | 2025-12-11 |
| mo-rickywong | Ricky Wong | Met Office | 2025-01-30 |
| mike-hobson | Mike Hobson | Met Office | 2025-12-17 |
| MatthewHambley | Matthew Hambley | Met Office | 2025-12-15 |
| mo-lottieturner | Lottie Turner | Met Office | 2025-12-16 |
| tommbendall | Thomas Bendall | Met Office | 2026-01-23 |
| yaswant | Yaswant Pradhan | Met Office | 2025-12-16 |
| stevemullerworth | Steve Mullerworth | Met Office | 2026-01-08 |
| harry-shepherd | Harry Shepherd | Met Office | 2026-01-08 |
| EdHone | Ed Hone | Met Office | 2026-01-09 |
| tom-j-h | Tom Hill | Met Office | 2026-01-19 |
| mo-alistairp | Alistair Pirrie | Met Office | 2026-01-12 |
| t00sa | Sam Clarke-Green | Met Office | 2026-01-27 |
| MetBenjaminWent | Benjamin Went | Met Office | 2026-01-30 |
| jcsmeto | James Cunningham-Smith | Met Office | 2026-02-06 |
| thomasmelvin | Thomas Melvin | Met Office | 2026-01-15 |
| ericaneininger | Erica Neininger | Met Office | 2026-03-02 |
| mo-lucy-gordon | Lucy Gordon | Met Office | 2026-03-18 |
| GitHub user | Real Name | Affiliation | Date |
| ------------------- | ---------------------- | ----------- | ---------- |
| andrewcoughtrie | Andrew Coughtrie | Met Office | 2025.12.12 |
| james-bruten-mo | James Bruten | Met Office | 2025-12-09 |
| jedbakerMO | Jed Baker | Met Office | 2025-12-29 |
| jennyhickson | Jenny Hickson | Met Office | 2025-12-10 |
| mo-marqh | Mark Hedley | Met Office | 2025-12-11 |
| mo-rickywong | Ricky Wong | Met Office | 2025-01-30 |
| mike-hobson | Mike Hobson | Met Office | 2025-12-17 |
| MatthewHambley | Matthew Hambley | Met Office | 2025-12-15 |
| mo-lottieturner | Lottie Turner | Met Office | 2025-12-16 |
| tommbendall | Thomas Bendall | Met Office | 2026-01-23 |
| yaswant | Yaswant Pradhan | Met Office | 2025-12-16 |
| stevemullerworth | Steve Mullerworth | Met Office | 2026-01-08 |
| harry-shepherd | Harry Shepherd | Met Office | 2026-01-08 |
| EdHone | Ed Hone | Met Office | 2026-01-09 |
| tom-j-h | Tom Hill | Met Office | 2026-01-19 |
| mo-alistairp | Alistair Pirrie | Met Office | 2026-01-12 |
| t00sa | Sam Clarke-Green | Met Office | 2026-01-27 |
| MetBenjaminWent | Benjamin Went | Met Office | 2026-01-30 |
| jcsmeto | James Cunningham-Smith | Met Office | 2026-02-06 |
| thomasmelvin | Thomas Melvin | Met Office | 2026-01-15 |
| ericaneininger | Erica Neininger | Met Office | 2026-03-02 |
| mo-lucy-gordon | Lucy Gordon | Met Office | 2026-03-18 |
| ukmo-juan-castillo | Juan M. Castillo | Met Office | 2026-03-24 |

4 changes: 3 additions & 1 deletion components/science/source/algorithm/sci_mapping_constants_mod.x90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,9 @@ contains
compute_sample_u_ops_kernel_type(u_lon_sample, &
u_lat_sample, &
u_up_sample, &
chi, panel_id) )
chi, panel_id, &
mesh%is_geometry_planar(), &
Copy link
Copy Markdown
Contributor

@tommbendall Thomas Bendall (tommbendall) Mar 24, 2026

Choose a reason for hiding this comment

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

I'm worried that this confuses the mesh's geometry and topology with the model's -- which is why you've had to add a conversion to get the enumerators in the kernel. Can't you pass the geometry and topology enumerators from base_mesh_config_mod as arguments to the kernel?

Copy link
Copy Markdown
Contributor

@tommbendall Thomas Bendall (tommbendall) Mar 24, 2026

Choose a reason for hiding this comment

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

I'm worried that this confuses the mesh's geometry and topology with the model's -- which is why you've had to add a conversion to get the enumerators in the kernel. Can't you pass the geometry and topology enumerators from base_mesh_config_mod as arguments to the kernel?

Oh I see, the point is that you don't want to use the base_mesh namelist so are trying to hack a way around it, since you can figure this out based on the mesh object.

Maybe what you've done is the only solution. I'm a bit anxious that there are two underlying problems:

  • we have two sets of duplicate geometry and topology information: both the mesh and the base_mesh namelist have independent definitions
  • the lfricinputs code is using lfric_core infrastructure that has some science options embedded into it, but doesn't want to use the science options

I'm also a bit concerned if we start having some kernels that get their geometry/topology information from the mesh, and some that get their information from the namelist, that we could end up with a lot of inconsistency and confusion

Copy link
Copy Markdown
Contributor

@tommbendall Thomas Bendall (tommbendall) Mar 24, 2026

Choose a reason for hiding this comment

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

Actually, I've now realised that these changes don't completely avoid the base_mesh_config, since you've had to import the enumerators into compute_sample_u_ops_kernel.

So now I think I'm back to my first suggestion: can't we have geometry and topology arguments to the kernel, and lfric2lfric can have its own logic based on the mesh to work out which geometry and topology enumerators to pass to the kernel?

Copy link
Copy Markdown
Author

@ukmo-juan-castillo Juan Manuel Castillo Sanchez (ukmo-juan-castillo) Mar 24, 2026

Choose a reason for hiding this comment

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

Yes, this is another option I was considering: passing the geometry and topology integer arguments to the kernel. This option is slightly more complicated because this information lies in the local_mesh and not the mesh object, but it is doable.

Ricky was looking into removing the base_mesh dependencies in the science code, and we just agreed that he will be reviewing this PR. Any comments are welcome though. I will wait for Ricky's opinion on what the best way to pass the parameters to the kernel is.

Copy link
Copy Markdown
Contributor

@tommbendall Thomas Bendall (tommbendall) Mar 24, 2026

Choose a reason for hiding this comment

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

Yes, this is another option I was considering: passing the geometry and topology integer arguments to the kernel. This option is slightly more complicated because this information lies in the local_mesh and not the mesh object, but it is doable.

Ricky was looking into removing the base_mesh dependencies in the science code, and we just agreed that he will be reviewing this PR. Any comments are welcome though. I will wait for Ricky's opinion on what the best way to pass the parameters to the kernel is.

If it is possible to pass through the geometry and topology integers, I think that would be a better solution here in isolation. But if there are other plans to remove the base_mesh namelist somehow (I'm not sure I'd understand why!) then I agree it would make sense for this PR to be consistent with those plans!

Copy link
Copy Markdown
Author

@ukmo-juan-castillo Juan Manuel Castillo Sanchez (ukmo-juan-castillo) Mar 25, 2026

Choose a reason for hiding this comment

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

Adding the problematic parameters to the subroutine parameters, is something that has been done in the past to fix lfric2lfric issues due to lack of flexibility of lfric_core. Why doing it here is a 'hacky' solution? What 'proper' solution do you propose? I would be happy to implement it.

Copy link
Copy Markdown
Contributor

@mo-rickywong Ricky Wong (mo-rickywong) Mar 25, 2026

Choose a reason for hiding this comment

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

Ricky Wong (Ricky Wong (@mo-rickywong)) Just to check, I think you would you accept the kernel taking the geometry and topology (from base_mesh_config) as arguments? That would at least be consistent with how we are trying to handle namelist options generally and wouldn't be a hack that is specific for lfric2lfric

The branch I'm working on does this (as intended for kernels) in the removal of namelist variables from module scope. The branch has already implemented this for compute_sample_u_ops_code. This PR conflicts and is a hack.

As it stands, if it is needed to progress, use it as a branch until compute_sample_u_ops_code is updated, but it will not go on trunk.

Copy link
Copy Markdown
Author

@ukmo-juan-castillo Juan Manuel Castillo Sanchez (ukmo-juan-castillo) Mar 25, 2026

Choose a reason for hiding this comment

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

We need a solution in the trunk. When do you reckon your branch will go to trunk? If it is going to take a long time, I would propose adding what you call the 'hack' in trunk and then you can modify it and do it properly in your branch. Is this reasonable? I can help you coding if you do not have much time, and if you give me details on how you want to do it.

Copy link
Copy Markdown
Collaborator

@stevemullerworth Steve Mullerworth (stevemullerworth) Mar 25, 2026

Choose a reason for hiding this comment

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

There was clearly a bit of misunderstanding in the above. I spoke with Ricky to clarify. Ricky was not saying that you should wait for his branch, he intended to say that you could use the solution that is in his branch, which is to use base_mesh_config_mod.

So, Ricky and Tom agree: base_mesh configurations of topology and geometry should be passed in rather than the values from the mesh object.

Copy link
Copy Markdown
Author

@ukmo-juan-castillo Juan Manuel Castillo Sanchez (ukmo-juan-castillo) Mar 25, 2026

Choose a reason for hiding this comment

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

Thank you, this is what we proposed to do, but despite asking several times I did not receive confirmation. I push the proposed new code, asking now if you would prefer extracting the geometry and topology parameters further in the subroutine call chain. I will update the trac log when it is ready, as well as the linked branch in lfric_apps.

mesh%is_topology_periodic()) )

if ( LPROF ) call stop_timing( id, 'runtime_constants.mapping' )

Expand Down
62 changes: 39 additions & 23 deletions ...onents/science/source/kernel/inter_function_space/sci_compute_sample_u_ops_kernel_mod.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@
module sci_compute_sample_u_ops_kernel_mod

use argument_mod, only : arg_type, func_type, &
GH_FIELD, GH_REAL, &
GH_OPERATOR, &
GH_FIELD, GH_SCALAR, GH_REAL,&
GH_LOGICAL, GH_OPERATOR, &
GH_INC, GH_READ, GH_WRITE, &
ANY_DISCONTINUOUS_SPACE_3, &
GH_BASIS, GH_DIFF_BASIS, &
CELL_COLUMN, GH_EVALUATOR, &
reference_element_data_type, &
normals_to_faces
use constants_mod, only : r_def, i_def
use constants_mod, only : r_def, i_def, l_def
use fs_continuity_mod, only : W2broken, W3, Wtheta, Wchi
use kernel_mod, only : kernel_type
use sci_chi_transform_mod, only : chi2llr
Expand All @@ -33,9 +33,10 @@ module sci_compute_sample_u_ops_kernel_mod
use reference_element_mod, only : W, S, N, E, T, B

use finite_element_config_mod, only: coord_system
use base_mesh_config_mod, only: geometry, topology, &
geometry_spherical, &
geometry_planar
use base_mesh_config_mod, only: geometry_spherical, &
geometry_planar, &
topology_fully_periodic, &
topology_non_periodic
use planet_config_mod, only: scaled_radius

implicit none
Expand All @@ -50,12 +51,14 @@ module sci_compute_sample_u_ops_kernel_mod
!>
type, public, extends(kernel_type) :: compute_sample_u_ops_kernel_type
private
type(arg_type) :: meta_args(5) = (/ &
arg_type(GH_OPERATOR, GH_REAL, GH_WRITE, W2broken, W3), &
arg_type(GH_OPERATOR, GH_REAL, GH_WRITE, W2broken, W3), &
arg_type(GH_OPERATOR, GH_REAL, GH_WRITE, W2broken, WTHETA), &
arg_type(GH_FIELD*3, GH_REAL, GH_READ, Wchi), &
arg_type(GH_FIELD, GH_REAL, GH_READ, ANY_DISCONTINUOUS_SPACE_3) &
type(arg_type) :: meta_args(7) = (/ &
arg_type(GH_OPERATOR, GH_REAL, GH_WRITE, W2broken, W3), &
arg_type(GH_OPERATOR, GH_REAL, GH_WRITE, W2broken, W3), &
arg_type(GH_OPERATOR, GH_REAL, GH_WRITE, W2broken, WTHETA), &
arg_type(GH_FIELD*3, GH_REAL, GH_READ, Wchi), &
arg_type(GH_FIELD, GH_REAL, GH_READ, ANY_DISCONTINUOUS_SPACE_3), &
arg_type(GH_SCALAR, GH_LOGICAL, GH_READ), &
arg_type(GH_SCALAR, GH_LOGICAL, GH_READ) &
/)
type(func_type) :: meta_funcs(1) = (/ &
func_type(Wchi, GH_BASIS, GH_DIFF_BASIS) &
Expand Down Expand Up @@ -89,6 +92,8 @@ module sci_compute_sample_u_ops_kernel_mod
!> @param[in] chi2 Coordinates in the second direction
!> @param[in] chi3 Coordinates in the third direction
!> @param[in] panel_id A field giving the ID for mesh panels
!> @param[in] is_planar_geometry A logical saying if the mesh geometry is planar
!> @param[in] is_periodic_topology A logical saying if the mesh topology is periodic
!> @param[in] ndf_w2b Number of DoFs per cell for broken W2
!> @param[in] ndf_w3 Number of DoFs per cell for W3
!> @param[in] ndf_wt Number of DoFs per cell for Wtheta
Expand All @@ -110,6 +115,8 @@ subroutine compute_sample_u_ops_code( col, nlayers, &
ncell_3d_3, u_rad_op, &
chi1, chi2, chi3, &
panel_id, &
is_planar_geometry, &
is_periodic_topology, &
ndf_w2b, ndf_w3, ndf_wt, &
ndf_chi, undf_chi, map_chi, &
chi_basis, chi_diff_basis, &
Expand Down Expand Up @@ -137,14 +144,16 @@ subroutine compute_sample_u_ops_code( col, nlayers, &
! Fields
real(kind=r_def), dimension(undf_pid), intent(in) :: panel_id
real(kind=r_def), dimension(undf_chi), intent(in) :: chi1, chi2, chi3
logical(kind=l_def), intent(in) :: is_planar_geometry
logical(kind=l_def), intent(in) :: is_periodic_topology

! Operators
real(kind=r_def), dimension(ncell_3d_1,ndf_w2b,ndf_w3), intent(inout) :: u_lon_op
real(kind=r_def), dimension(ncell_3d_2,ndf_w2b,ndf_w3), intent(inout) :: u_lat_op
real(kind=r_def), dimension(ncell_3d_3,ndf_w2b,ndf_wt), intent(inout) :: u_rad_op

! Internal variables
integer(kind=i_def) :: df_w2, df_wt, df_chi, k, ipanel, cell_3d
integer(kind=i_def) :: df_w2, df_wt, df_chi, k, ipanel, cell_3d, topology
real(kind=r_def), dimension(3,3,ndf_w2b) :: jacobian, jac_inv
real(kind=r_def), dimension(ndf_w2b) :: dj
real(kind=r_def), dimension(3) :: llr, X_vector, Y_vector, Z_vector
Expand All @@ -157,10 +166,15 @@ subroutine compute_sample_u_ops_code( col, nlayers, &

ipanel = int(panel_id(map_pid(1)), i_def)

if (is_periodic_topology) then
topology = topology_fully_periodic
else
topology = topology_non_periodic
end if

! For spherical geometry, need to rotate from (lon,lat,r) components
! For planar geometry, components should already be in (X,Y,Z) coordinates
select case ( geometry )
case ( geometry_planar )
if ( is_planar_geometry ) then

X_vector = (/ 1.0_r_def, 0.0_r_def, 0.0_r_def /)
Y_vector = (/ 0.0_r_def, 1.0_r_def, 0.0_r_def /)
Expand All @@ -180,9 +194,10 @@ subroutine compute_sample_u_ops_code( col, nlayers, &
chi3_e(df_chi) = chi3(map_chi(df_chi) + k)
end do

call coordinate_jacobian(coord_system, geometry, topology, scaled_radius, &
ndf_chi, ndf_w2b, chi1_e, chi2_e, chi3_e, &
ipanel, chi_basis, chi_diff_basis, jacobian, dj)
call coordinate_jacobian(coord_system, geometry_planar, topology, &
scaled_radius, ndf_chi, ndf_w2b, chi1_e, &
chi2_e, chi3_e, ipanel, chi_basis, &
chi_diff_basis, jacobian, dj)
call coordinate_jacobian_inverse(ndf_w2b, jacobian, dj, jac_inv)

! X and Y components contribute equally to all W2 DoFs
Expand Down Expand Up @@ -210,7 +225,7 @@ subroutine compute_sample_u_ops_code( col, nlayers, &

end do

case ( geometry_spherical )
else

lon_vector_llr = (/ 1.0_r_def, 0.0_r_def, 0.0_r_def /)
lat_vector_llr = (/ 0.0_r_def, 1.0_r_def, 0.0_r_def /)
Expand All @@ -230,9 +245,10 @@ subroutine compute_sample_u_ops_code( col, nlayers, &
chi3_e(df_chi) = chi3(map_chi(df_chi) + k)
end do

call coordinate_jacobian(coord_system, geometry, topology, scaled_radius, &
ndf_chi, ndf_w2b, chi1_e, chi2_e, chi3_e, &
ipanel, chi_basis, chi_diff_basis, jacobian, dj)
call coordinate_jacobian(coord_system, geometry_spherical, topology, &
scaled_radius, ndf_chi, ndf_w2b, chi1_e, &
chi2_e, chi3_e, ipanel, chi_basis, &
chi_diff_basis, jacobian, dj)
call coordinate_jacobian_inverse(ndf_w2b, jacobian, dj, jac_inv)

! Convert (lon,lat,r) vectors into (X,Y,Z) components
Expand Down Expand Up @@ -284,7 +300,7 @@ subroutine compute_sample_u_ops_code( col, nlayers, &

end do

end select
end if

! Enforce boundary condition at bottom and top
cell_3d = 1 + (col-1)*nlayers
Expand Down
9 changes: 8 additions & 1 deletion ...nts/science/unit-test/kernel/inter_function_space/compute_sample_u_ops_kernel_mod_test.pf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

module compute_sample_u_ops_kernel_mod_test

use constants_mod, only : i_def, r_def
use constants_mod, only : i_def, r_def, l_def
use reference_element_mod, only : W, S, E, N, B, T
use funit

Expand Down Expand Up @@ -147,6 +147,9 @@ contains
real(r_def), allocatable :: u_lat_op(:,:,:)
real(r_def), allocatable :: u_up_op(:,:,:)

logical(l_def) :: is_planar_geometry
logical(l_def) :: is_periodic_topology

! ------------------------------------------------------------------------ !
! Make mesh and function space details
! ------------------------------------------------------------------------ !
Expand Down Expand Up @@ -218,6 +221,8 @@ contains
u_up_op(:,:,:) = 0.0_r_def
u_w2_data(:) = 0.0_r_def
u_w2b_data(:) = 0.0_r_def
is_planar_geometry = .false.
is_periodic_topology = .false.

! Set values for components, that vary with column and layer
do k = 0, nlayers - 1
Expand Down Expand Up @@ -275,6 +280,8 @@ contains
chi_data(:,2), &
chi_data(:,3), &
panel_id_data, &
is_planar_geometry, &
is_periodic_topology, &
ndf_w2b, ndf_w3, ndf_wt, &
ndf_wchi, undf_wchi, map_wchi(:,cell), &
basis_wchi, diff_basis_wchi, &
Expand Down
Loading