Documentaiton update - Kernels #1447
Conversation
- Adds documentation for `assembly::kernels`
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive Doxygen documentation for the specfem::assembly::kernels module, which manages misfit kernels for seismic inversion in spectral element simulations.
Key Changes:
- Added detailed class-level documentation for 2D and 3D kernel template specializations with physical context and usage examples
- Refactored data access functions from inline definitions into separate header files organized by operation type (load/store/add) and execution space (host/device)
- Updated RST documentation to include dimension-specific implementations and corrected the Doxygen group reference
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
docs/sections/api/specfem/assembly/kernels/kernels.rst |
Added documentation sections for dimension-specific kernel implementations and updated group reference |
core/specfem/assembly/kernels/dim3/kernels.hpp |
Enhanced documentation with comprehensive class overview, detailed constructor descriptions, and usage examples |
core/specfem/assembly/kernels/dim2/kernels.hpp |
Enhanced documentation with comprehensive class overview, detailed constructor descriptions, and usage examples |
core/specfem/assembly/kernels/data_access/store_on_host.hpp |
New file containing host-side store function with documentation |
core/specfem/assembly/kernels/data_access/store_on_device.hpp |
New file containing device-side store function with documentation |
core/specfem/assembly/kernels/data_access/load_on_host.hpp |
New file containing host-side load function with documentation |
core/specfem/assembly/kernels/data_access/load_on_device.hpp |
New file containing device-side load function with documentation |
core/specfem/assembly/kernels/data_access/add_on_host.hpp |
New file containing host-side accumulation function with documentation |
core/specfem/assembly/kernels/data_access/add_on_device.hpp |
New file containing device-side accumulation function with documentation |
core/specfem/assembly/kernels.hpp |
Refactored to move inline function definitions to separate headers and added improved documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * const int specfem::point::index<...> index = ...; // Current element and GLL | ||
| * point const specfem::point::kernels<...> point_kernels = ...; |
There was a problem hiding this comment.
The code example contains invalid C++ syntax. Line 36 has: const int specfem::point::index<...> index = ...; which is incorrect. The const int should be removed - it should just be const specfem::point::index<...> index = ...; or const auto index = ...;.
| * const int specfem::point::index<...> index = ...; // Current element and GLL | |
| * point const specfem::point::kernels<...> point_kernels = ...; | |
| * const auto index = ...; // Current element and GLL | |
| * const specfem::point::kernels<...> point_kernels = ...; |
| // clang-format off | ||
| * @code | ||
| * // Example: Accumulate elastic kernels during adjoint simulation | ||
| * const int specfem::point::index<...> index = ...; // Current element and GLL |
There was a problem hiding this comment.
The code example contains invalid C++ syntax. Line 35 has: const int specfem::point::index<...> index = ...; which is incorrect. The const int should be removed - it should just be const specfem::point::index<...> index = ...; or const auto index = ...;.
| * const int specfem::point::index<...> index = ...; // Current element and GLL | |
| * const specfem::point::index<...> index = ...; // Current element and GLL |
| kernels(const int nspec, const int ngllz, const int nglly, const int ngllx, | ||
| const specfem::assembly::element_types<dimension_tag> &element_types); |
There was a problem hiding this comment.
The documentation states this is a constructor, but the order is inconsistent with typical C++ documentation patterns. The parameterized constructor documentation appears before the default constructor, but the actual declarations show the default constructor (line 76) after the parameterized constructor (lines 69-70). Consider documenting them in the order they appear in the code.
| * const int specfem::point::index<...> index = ...; // Current element and GLL | ||
| * point const specfem::point::kernels<...> point_kernels = ...; |
There was a problem hiding this comment.
The code example contains invalid C++ syntax. Line 34 has: const int specfem::point::index<...> index = ...; which is incorrect. The const int should be removed - it should just be const specfem::point::index<...> index = ...; or const auto index = ...;.
| * const int specfem::point::index<...> index = ...; // Current element and GLL | |
| * point const specfem::point::kernels<...> point_kernels = ...; | |
| * const specfem::point::index<...> index = ...; // Current element and GLL point | |
| * const specfem::point::kernels<...> point_kernels = ...; |
| * | ||
| * @ingroup KernelsDataAccess |
There was a problem hiding this comment.
The @ingroup tag is inconsistently placed. In this file it appears on line 18 after the brief description, but in other similar files (store_on_host.hpp, store_on_device.hpp, add_on_device.hpp, add_on_host.hpp) it appears on line 20 or later. For consistency, the @ingroup tag should be placed immediately after the @brief tag as it is in the other files.
| * | |
| * @ingroup KernelsDataAccess | |
| * @ingroup KernelsDataAccess | |
| * |
| kernels(const int nspec, const int ngllz, const int ngllx, | ||
| const specfem::assembly::element_types<dimension_tag> &element_types); |
There was a problem hiding this comment.
The documentation states this is a constructor, but the order is inconsistent with typical C++ documentation patterns. The parameterized constructor documentation appears before the default constructor, but the actual declarations show the default constructor (line 74) after the parameterized constructor (lines 67-68). Consider documenting them in the order they appear in the code.
|
|
||
| /** | ||
| * @brief Construct a new kernels object | ||
| * @brief Default destructor |
There was a problem hiding this comment.
The documentation says "Default destructor" but this is actually the default constructor (kernels() = default;). It should be labeled as "Default constructor".
| * @brief Default destructor | |
| * @brief Default constructor |
| * const int specfem::point::index<...> index = ...; // Current element and GLL | ||
| * point const specfem::point::kernels<...> point_kernels = ...; |
There was a problem hiding this comment.
The code example contains invalid C++ syntax. Line 34 has: const int specfem::point::index<...> index = ...; which is incorrect. The const int should be removed - it should just be const specfem::point::index<...> index = ...; or const auto index = ...;.
| * const int specfem::point::index<...> index = ...; // Current element and GLL | |
| * point const specfem::point::kernels<...> point_kernels = ...; | |
| * const specfem::point::index<...> index = ...; // Current element and GLL point | |
| * const specfem::point::kernels<...> point_kernels = ...; |
| // clang-format off | ||
| * @code | ||
| * // Example: Load elastic kernels during adjoint simulation | ||
| * const int specfem::point::index<...> index = ...; // Current element and GLL |
There was a problem hiding this comment.
The code example contains invalid C++ syntax. Line 33 has: const int specfem::point::index<...> index = ...; which is incorrect. The const int should be removed - it should just be const specfem::point::index<...> index = ...; or const auto index = ...;.
| * const int specfem::point::index<...> index = ...; // Current element and GLL | |
| * const specfem::point::index<...> index = ...; // Current element and GLL |
| // clang-format off | ||
| * @code | ||
| * // Example: Load elastic kernels during adjoint simulation | ||
| * const int specfem::point::index<...> index = ...; // Current element and GLL |
There was a problem hiding this comment.
The code example contains invalid C++ syntax. Line 35 has: const int specfem::point::index<...> index = ...; which is incorrect. The const int should be removed - it should just be const specfem::point::index<...> index = ...; or const auto index = ...;.
| * const int specfem::point::index<...> index = ...; // Current element and GLL | |
| * const auto index = /* specfem::point::index<...> */ ...; // Current element and GLL |
Description
Please describe the changes/features in this pull request.
assembly::kernelsIssue Number
If there is an issue created for these changes, link it here
Checklist
Please make sure to check developer documentation on specfem docs.