Skip to content

Conversation

@Rohit-Kakodkar
Copy link
Collaborator

Description

Please describe the changes/features in this pull request.

  • Adds tests for acoustic 3D CR

Issue Number

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

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

- Adds tests for acoustic 3D CR
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds new unit tests to cover 3D acoustic CR (constitutive relations) behavior in the existing medium physics test suite.

Changes:

  • Add 3D acoustic isotropic stress computation tests.
  • Add 3D acoustic isotropic source contribution tests.
  • Add 3D acoustic isotropic mass matrix component test and wire new test files into the unit-test CMake targets.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
tests/unit-tests/medium/stress/dim3/acoustic.cpp New 3D acoustic stress tests (basic + zero-derivatives).
tests/unit-tests/medium/source/dim3/acoustic.cpp New 3D acoustic source contribution tests (basic + zero STF).
tests/unit-tests/medium/mass_matrix/dim3/acoustic.cpp New 3D acoustic mass-matrix component test.
tests/unit-tests/CMakeLists.txt Adds the new 3D acoustic test sources to existing test executables.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


PointAccelerationType expected_acceleration;
expected_acceleration(0) =
point_source.stf(0) * point_source.lagrange_interpolant(0) / kappa;
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

The expected sign here (+ stf * L / kappa) is inconsistent with the existing 2D acoustic convention, where impl_compute_source_contribution applies a negative sign (see core/specfem/medium/dim2/acoustic/isotropic/source.hpp). The 3D acoustic implementation currently uses a positive sign (core/specfem/medium/dim3/acoustic/isotropic/source.hpp), so either the 3D implementation or this test (and/or the 2D implementation) likely has the wrong sign convention. Please clarify the intended physical/sign convention and align dim2/dim3 (update the test expectation and/or the dim3 implementation accordingly).

Suggested change
point_source.stf(0) * point_source.lagrange_interpolant(0) / kappa;
-point_source.stf(0) * point_source.lagrange_interpolant(0) / kappa;

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@copilot can you create a PR that updates this as well as core/specfem/medium/dim3/acoustic/isotropic/source.hpp to match this?

Comment on lines +23 to +25
const type_real kappa = 10.0;
const PointPropertiesType properties(kappa);

Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

PointPropertiesType properties(kappa); uses the single-value ctor and sets both rho_inverse and kappa to the same value. Even though compute_source_contribution() only uses kappa today, this is ambiguous in a properties container that conceptually holds multiple values. Prefer passing explicit (rho_inverse, kappa) (mirroring the 2D acoustic source tests) to keep the test intent unambiguous.

Copilot uses AI. Check for mistakes.
Comment on lines +20 to +21

const PointPropertiesType properties(kappa);
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

PointPropertiesType properties(kappa); invokes the single-value ctor and sets both rho_inverse and kappa to kappa. For a multi-property container this is easy to misread and can hide bugs if mass_matrix_component() later starts using additional properties. Consider constructing with explicit (rho_inverse, kappa) values to make the test setup precise.

Suggested change
const PointPropertiesType properties(kappa);
const type_real rho_inverse = static_cast<type_real>(1.0);
const PointPropertiesType properties(rho_inverse, kappa);

Copilot uses AI. Check for mistakes.
std::ostringstream message;
message << "Mass matrix is not equal to expected value: " << mass_matrix(0)
<< " != " << expected_mass_matrix(0);
EXPECT_TRUE(mass_matrix == expected_mass_matrix);
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

The message stream is built but never attached to the failing assertion, so a failure won’t print the helpful diagnostics. Either append << message.str() to the EXPECT_TRUE(...) (as done in the stress/source tests) or remove the unused message construction.

Suggested change
EXPECT_TRUE(mass_matrix == expected_mass_matrix);
EXPECT_TRUE(mass_matrix == expected_mass_matrix) << message.str();

Copilot uses AI. Check for mistakes.
Comment on lines +21 to +23
const type_real rho_inverse = 2.0;
const PointPropertiesType properties(rho_inverse);

Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

PointPropertiesType properties(rho_inverse); calls the single-value point_container ctor, which sets all acoustic properties (including kappa) to rho_inverse. This makes the test less clear and could become wrong if compute_stress() or related helpers start using kappa for acoustic 3D. Prefer constructing with explicit (rho_inverse, kappa) values (as done in the 2D acoustic stress tests).

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI commented Feb 10, 2026

@lsawade I've opened a new pull request, #1642, to work on those changes. Once the pull request is ready, I'll request review from you.

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.

4 participants