docs: Adding Code rules page #3915
Conversation
MelReyCG
requested review from
CusiniM,
OmarDuran,
castelletto1,
dkachuma,
herve-gross,
jhuang2601,
paveltomin and
rrsettgast
as code owners
jafranc
left a comment
jafranc
left a comment
There was a problem hiding this comment.
Looks great ! Maybe there is the work of 2 docs there (or more 😄 )
| Use GEOS tensor types for geometric and mechanical calculations: | ||
|
|
||
| .. list-table:: GEOS Tensor Types | ||
| :header-rows: 1 | ||
| :widths: 30 70 | ||
|
|
||
| * - Type | ||
| - Description | ||
| * - ``R1Tensor`` | ||
| - 3D vector (real64) | ||
| * - ``R1Tensor32`` | ||
| - 3D vector (real32) | ||
| * - ``R2SymTensor`` | ||
| - Symmetric 6-component tensor (Voigt notation) |
There was a problem hiding this comment.
I feel like we can expand a bit on why tensor are in use and not their vector counter part
There was a problem hiding this comment.
Do you have a suggestion? I have an idea but I think not precise enough.
| setApplyDefaultValue( 1.0e-6 ). | ||
| setDescription( "Initial time step value. This parameter controls " | ||
| "the starting time increment for the simulation. " | ||
| "Valid range: (0, 1e-3]. See User Guide Chapter 5." ); |
There was a problem hiding this comment.
For later, should we think of adding an optional setRange to Wrappers ?
There was a problem hiding this comment.
I totally agree, along with "unit" type. This is the direction I want to promote.
| - **Use GoogleTest framework**. | ||
| - In GEOS, the goal of unit test is never to take position on the validity of physical results (this is the point of integrated-tests). | ||
| - Place tests in ``src/coreComponents/<component>/tests/``. | ||
| - Test GPU code paths if applicable (use ``#ifdef GEOS_USE_DEVICE``). |
There was a problem hiding this comment.
Would look nice in the example too
| wrappedStats.stats().m_productionRate ); | ||
|
|
||
| // statistics CSV output (only if enabled and from rank 0) | ||
| if( m_writeCSV > 0 && MpiWrapper::commRank() == 0 ) |
There was a problem hiding this comment.
Why not wrap this in a MACRO and advise to use it ?
| Additional Validation Guidelines | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| - Use ``setInputFlag()`` to mark parameters as ``REQUIRED`` or ``OPTIONAL`` | ||
| - Use ``setApplyDefaultValue()`` to provide sensible defaults | ||
| - Use ``setRTTypeName()`` for runtime type validation (e.g., ``rtTypes::CustomTypes::positive``) | ||
| - Document valid ranges in ``setDescription()`` |
There was a problem hiding this comment.
Shouldn't it be in the Wrapper section ?
| Caliper Integration | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| Use ``GEOS_MARK_FUNCTION`` and ``Timer`` for performance tracking for the main computation functions / scopes. |
There was a problem hiding this comment.
and GEOS_CALIPER_MARK_SCOPE() then ?
| Provide Views to Arrays | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| - When possible, prefer provide views to arrays (to const data if possible). | ||
| - This **must** be done especially for RAJA kernels, and views must be **captured by value**. | ||
|
|
||
| The rule is generalizable to ``string_view`` for strings, but not applicable in GPU context. | ||
|
|
||
| Why Use Views? | ||
|
|
||
| - **No memory allocation:** Views are lightweight references | ||
| - **Mutability correctness:** Can provide ``const`` read-only access to inner data | ||
| - **GPU compatibility:** Views work seamlessly on device | ||
|
|
||
| .. dropdown:: Example: Views for arrays | ||
| :icon: code | ||
|
|
||
| .. code-block:: c++ | ||
|
|
||
| // BAD - Creates a copy | ||
| array1d< real64 > copy = originalArray; | ||
|
|
||
| // GOOD - Creates a view (lightweight) | ||
| arrayView1d< real64 > view = originalArray.toView(); | ||
|
|
||
| // GOOD - Const view for read-only access | ||
| arrayView1d< real64 const > constView = originalArray.toViewConst(); | ||
|
|
There was a problem hiding this comment.
This might be moved above for more visibility ?
|
Thanks @jafranc for your feedback, I'll take them into account. |
| Use ``GEOS_WARNING*`` macros when: | ||
|
|
||
| - An issue is detected but **simulation can continue** | ||
| - Simulation may be **affected but not completly invalid** |
There was a problem hiding this comment.
| - Simulation may be **affected but not completly invalid** | |
| - Simulation may be **affected but outcomes remain valid** |
There was a problem hiding this comment.
I'm not sure of what to write here.
Warning in GEOS: We know that there may an issue, but simulation can continue to proceed, and results may be right.
|
|
||
| The rule is generalizable to ``string_view`` for strings, but not applicable in GPU context. | ||
|
|
||
| Why Use Views? |
There was a problem hiding this comment.
| Why Use Views? | |
| **Rationale:** | |
There was a problem hiding this comment.
Isn't that less meaningful?
There was a problem hiding this comment.
I do not love "Rationale" but it has been used before in the document. I just put it here again for consistency. Feel free to revert, but then keep it consistent throughout.
| **Minimize mutable global states when possible.** | ||
| Prefer passing context explicitly. | ||
|
|
||
| Why to avoid globals: |
There was a problem hiding this comment.
| Why to avoid globals: | |
| **Rationale:** | |
There was a problem hiding this comment.
Isn't that less meaningful?
GEOS Code Rules
Aims at writing down the coding standards for GEOS, focusing on type safety, error handling, parallelism, and performance. Key principles include:
These rules ensure code quality, consistency, and maintainability across the GEOS codebase.