Skip to content

Conversation

@int-ptr-ptr
Copy link
Collaborator

Description

Please describe the changes/features in this pull request.

Issue Number

If there is an issue created for these changes, link it here

#1258

Checklist

Please make sure to check developer documentation on specfem docs.

  • I ran the code through pre-commit to check style
  • THE DOCUMENTATION BUILDS WITHOUT WARNINGS/ERRORS
  • I have added labels to the PR (see right hand side of the PR page)
  • My code passes all the integration tests
  • I have added sufficient unittests to test my changes
  • I have added/updated documentation for the changes I am proposing
  • I have updated CMakeLists to ensure my code builds
  • My code builds across all platforms

@int-ptr-ptr int-ptr-ptr linked an issue Jan 6, 2026 that may be closed by this pull request
2 tasks
Comment on lines 136 to 144
for (int ielem = 0; ielem < virtual_chunk_size; ++ielem) {
for (int ipoint = 0; ipoint < TransferFunction2D::nquad_intersection;
++ipoint) {
for (int icomp = 0; icomp < ncomp_self; ++icomp) {
computed_coupling_function(iedge_start + ielem, ipoint, icomp) =
CCF(ielem, ipoint, icomp);
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's happening here. Shouldn't passing the subbview already store it in computed_coupling_function. Are you doing a = a; here ?

Comment on lines 148 to 152
typename decltype(
computed_coupling_function)::HostMirror h_computed_coupling_function =
Kokkos::create_mirror_view(computed_coupling_function);

Kokkos::deep_copy(h_computed_coupling_function, computed_coupling_function);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
typename decltype(
computed_coupling_function)::HostMirror h_computed_coupling_function =
Kokkos::create_mirror_view(computed_coupling_function);
Kokkos::deep_copy(h_computed_coupling_function, computed_coupling_function);
const auto h_computed_coupling_function = Kokkos::create_mirror_view_and_copy(computed_coupling_function);

for (int ipoint = 0; ipoint < TransferFunction2D::nquad_intersection;
++ipoint) {
for (int icomp = 0; icomp < ncomp_self; ++icomp) {
const type_real got = computed_coupling_function(ielem, ipoint, icomp);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const type_real got = computed_coupling_function(ielem, ipoint, icomp);
const type_real got = h_computed_coupling_function(ielem, ipoint, icomp);

Comment on lines 31 to 36
/**
* @brief Concatenates k functions into an array-valued function.
*
*/
template <typename... AnalyticalFunctions>
struct Vectorized : AnalyticalFunctionType {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs more description

Comment on lines 74 to 78
/**
* @brief Sums k functions.
*
*/
template <typename... AnalyticalFunctions> struct Sum : AnalyticalFunctionType {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

Copy link
Collaborator

@lsawade lsawade left a comment

Choose a reason for hiding this comment

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

I think this should be reworked into a parameterized test. It seems overengineered. TransferFunctionTestTypes2D_AcousticElastic_Natural could just be a set of parameters from a map that initialize whatever needs to be tested. But maybe I'm missing something.

Copy link
Collaborator

@lsawade lsawade left a comment

Choose a reason for hiding this comment

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

See PR

@int-ptr-ptr int-ptr-ptr changed the base branch from devel to issue-1554 January 18, 2026 21:19
@int-ptr-ptr
Copy link
Collaborator Author

Note

I changed the base to issue-1554 for a better git diff.

@icui icui deleted the branch PrincetonUniversity:devel January 19, 2026 23:33
@icui icui closed this Jan 19, 2026
@int-ptr-ptr int-ptr-ptr reopened this Jan 19, 2026
@int-ptr-ptr int-ptr-ptr changed the base branch from issue-1554 to devel January 19, 2026 23:39
Comment on lines 134 to 144
// for (int ielem = 0; ielem < virtual_chunk_size; ++ielem) {
// for (int ipoint = 0; ipoint <
// TransferFunction2D::nquad_intersection;
// ++ipoint) {
// for (int icomp = 0; icomp < ncomp_self; ++icomp) {
// computed_coupling_function(iedge_start + ielem, ipoint, icomp)
// =
// CCF(ielem, ipoint, icomp);
// }
// }
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// for (int ielem = 0; ielem < virtual_chunk_size; ++ielem) {
// for (int ipoint = 0; ipoint <
// TransferFunction2D::nquad_intersection;
// ++ipoint) {
// for (int icomp = 0; icomp < ncomp_self; ++icomp) {
// computed_coupling_function(iedge_start + ielem, ipoint, icomp)
// =
// CCF(ielem, ipoint, icomp);
// }
// }
// }

@Rohit-Kakodkar Rohit-Kakodkar self-requested a review January 22, 2026 14:45
Copy link
Collaborator

@Rohit-Kakodkar Rohit-Kakodkar left a comment

Choose a reason for hiding this comment

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

Minor changes.

Comment on lines 225 to 226
specfem::test_fixture::IntersectionFunction2D<ExpectedSolutionInit>
expected_solution{ ExpectedSolutionInit{} };
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will not be generalizable. For example, a case where we manually compute the expected solution for a simplied transfer/intersection function, you'd have to write another runner. I would suggest creating a compute_expected_solution function and write a specialization that calls this initializer, when you know the solution analytically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure what you mean, here. IntersectionFunction2D is the array itself. The compute_expected_solution is embedded within the initializer, which gives the 3D array of type_real.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure what you mean by compute_expected_solution is embedded within the initializer. What would happen if the expected solution init is not a function of type analytical function?

I would have thought that in a general case you'd compute the expected solution using a serial loop, similar to how we do it in the gradient test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See PR int-ptr-ptr#5

Copy link
Collaborator

@Rohit-Kakodkar Rohit-Kakodkar Jan 23, 2026

Choose a reason for hiding this comment

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

I'm not sure why the intersection function is needed. The purpose of adding the mock classes was to supply input for the functions under test. The intersection function seems to be attempting to mock the return type. Am I overlooking something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IntersectionFunction is an input type for the coupling integrals (and normal vector).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah okay, Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah okay, Thanks!

@Rohit-Kakodkar Rohit-Kakodkar merged commit e7e98b3 into PrincetonUniversity:devel Jan 27, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add test for NCI compute_coupling

4 participants