Skip to content

Conversation

@Rohit-Kakodkar
Copy link
Collaborator

Description

Please describe the changes/features in this pull request.

Adds documentation for assembly fields

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 documentation for assembly fields
Base automatically changed from issue-1387-part-5 to devel December 2, 2025 18:04
Copilot AI review requested due to automatic review settings December 4, 2025 18:51
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

This PR adds comprehensive Doxygen documentation for the assembly fields module in SPECFEM++, including detailed descriptions for field storage classes, simulation field containers, and data access functions.

Key changes:

  • Added complete documentation for field_impl, base_field, and field management classes
  • Enhanced RST documentation structure with new files for implementation details
  • Added usage examples and detailed parameter descriptions for all data access functions (load, store, add operations)

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
docs/sections/api/specfem/assembly/fields/fields.rst Updated RST structure to include dimension-specific implementations and corrected doxygen group reference
docs/sections/api/specfem/assembly/fields/field_impl.rst New documentation file for field implementation details
docs/sections/api/specfem/assembly/fields/base_field.rst New documentation file for base field class
core/specfem/assembly/fields/impl/field_impl.hpp Added comprehensive class and method documentation for field implementation
core/specfem/assembly/fields/fields.hpp Enhanced documentation for the main fields container with usage examples
core/specfem/assembly/fields/dim3/simulation_field.hpp Added detailed 3D simulation field documentation
core/specfem/assembly/fields/dim2/simulation_field.hpp Added detailed 2D simulation field documentation
core/specfem/assembly/fields/data_access/store_on_host.hpp Expanded documentation with usage examples for host-side storage
core/specfem/assembly/fields/data_access/store_on_device.hpp Expanded documentation with usage examples for device-side storage
core/specfem/assembly/fields/data_access/load_on_host.hpp Expanded documentation with usage examples for host-side loading
core/specfem/assembly/fields/data_access/load_on_device.hpp Expanded documentation with usage examples for device-side loading
core/specfem/assembly/fields/data_access/atomic_add_on_device.hpp Enhanced atomic operation documentation with usage examples
core/specfem/assembly/fields/data_access/add_on_device.hpp Added defgroup definition and improved documentation with examples
core/specfem/assembly/fields.hpp Added brief documentation for simulation_field forward declaration

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

Comment on lines +77 to +79
* displacement(0) = computed_disp_x;
* displacement(1) = computed_disp_z;
* velocity(0) = computed_vel_x;
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

In the code example, the variable name displacement is used but the accessor is named disp. The example should be consistent: either use disp(0) = ... or rename the accessor variable to displacement.

Suggested change
* displacement(0) = computed_disp_x;
* displacement(1) = computed_disp_z;
* velocity(0) = computed_vel_x;
* disp(0) = computed_disp_x;
* disp(1) = computed_disp_z;
* vel(0) = computed_vel_x;

Copilot uses AI. Check for mistakes.
Comment on lines +105 to +106
* auto disp_x = displacement(0);
* auto disp_z = displacement(1);
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

In the code example, the variable name displacement is used to access values but the accessor is named disp. The example should be consistent: use disp(0) instead of displacement(0).

Suggested change
* auto disp_x = displacement(0);
* auto disp_z = displacement(1);
* auto disp_x = disp(0);
* auto disp_z = disp(1);

Copilot uses AI. Check for mistakes.
Comment on lines +83 to +84
* displacement(0) += delta_disp_x;
* displacement(1) += delta_disp_z;
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

In the code example, the variable name displacement is used to set values but the accessor is named disp. The example should be consistent: use disp(0) += ... instead of displacement(0) += ....

Suggested change
* displacement(0) += delta_disp_x;
* displacement(1) += delta_disp_z;
* disp(0) += delta_disp_x;
* disp(1) += delta_disp_z;

Copilot uses AI. Check for mistakes.
Comment on lines +73 to +74
* displacement(0) += delta_disp_x;
* displacement(1) += delta_disp_z;
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

In the code example, the variable name displacement is used to update values but the accessor is named disp. The example should be consistent: use disp(0) += ... instead of displacement(0) += ....

Suggested change
* displacement(0) += delta_disp_x;
* displacement(1) += delta_disp_z;
* disp(0) += delta_disp_x;
* disp(1) += delta_disp_z;

Copilot uses AI. Check for mistakes.
Comment on lines +163 to +165
* auto elastic_field =
* field.get_field<specfem::element::medium_tag::elastic_psv>(); auto
* displacement = elastic_field.displacement;
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

In the code example, the variable name displacement is used to access field values but the accessor variable is named elastic_field. The example should use elastic_field.get_field() or similar to properly demonstrate field access.

Suggested change
* auto elastic_field =
* field.get_field<specfem::element::medium_tag::elastic_psv>(); auto
* displacement = elastic_field.displacement;
* auto elastic_field = field.get_field<specfem::element::medium_tag::elastic_psv>();
* auto displacement = elastic_field.displacement;
* // Now you can use displacement for further computations

Copilot uses AI. Check for mistakes.
Comment on lines +104 to +105
* std::cout << "Displacement X: " << displacement(0) << std::endl;
* std::cout << "Displacement Z: " << displacement(1) << std::endl;
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

In the code example, the variable name displacement is used in the std::cout statement but the accessor is named disp. The example should be consistent: use disp(0) instead of displacement(0).

Suggested change
* std::cout << "Displacement X: " << displacement(0) << std::endl;
* std::cout << "Displacement Z: " << displacement(1) << std::endl;
* std::cout << "Displacement X: " << disp(0) << std::endl;
* std::cout << "Displacement Z: " << disp(1) << std::endl;

Copilot uses AI. Check for mistakes.
Comment on lines +166 to +168
* auto elastic_field =
* field.get_field<specfem::element::medium_tag::elastic>(); auto displacement
* = elastic_field.displacement;
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

In the code example, the variable name displacement is used but is not defined in the example. The example should show how to properly access displacement from elastic_field.

Suggested change
* auto elastic_field =
* field.get_field<specfem::element::medium_tag::elastic>(); auto displacement
* = elastic_field.displacement;
* auto const& elastic_field =
* field.get_field<specfem::element::medium_tag::elastic>();
* auto const& displacement = elastic_field.displacement;

Copilot uses AI. Check for mistakes.
Comment on lines +43 to +44
* Public interface for accumulating values into simulation fields on default compute device (GPU when compiled with GPU enabled)
* devices. This function provides a unified interface for adding values to
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

This line appears incomplete with "devices. This function" - there's an awkward break. The sentence should flow directly without the word "devices" appearing standalone.

Suggested change
* Public interface for accumulating values into simulation fields on default compute device (GPU when compiled with GPU enabled)
* devices. This function provides a unified interface for adding values to
* Public interface for accumulating values into simulation fields on the default compute device (GPU when compiled with GPU enabled). This function provides a unified interface for adding values to

Copilot uses AI. Check for mistakes.
using displacement_base_type::get_value;
using mass_inverse_base_type::get_value;
using velocity_base_type::get_value;
// Import get_value methods from all base field types for unified access
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The comment "Import get_value methods from all base field types for unified access" could be more precise. Consider: "Bring get_value methods from base classes into scope for unified field access".

Suggested change
// Import get_value methods from all base field types for unified access
// Bring get_value methods from base classes into scope for unified field access

Copilot uses AI. Check for mistakes.
Comment on lines +99 to +100
* displacement(0) = initial_disp_x;
* displacement(1) = initial_disp_z;
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

In the code example, the variable name displacement is used but the accessor is named disp. The example should be consistent: either use disp(0) = ... or rename the accessor variable to displacement.

Suggested change
* displacement(0) = initial_disp_x;
* displacement(1) = initial_disp_z;
* disp(0) = initial_disp_x;
* disp(1) = initial_disp_z;

Copilot uses AI. Check for mistakes.
@Rohit-Kakodkar Rohit-Kakodkar requested a review from icui December 8, 2025 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants