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 😄 )
| 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 ?
There was a problem hiding this comment.
It feels strange to me, as the wrapper section is for documentation requirement. My goal here is to precise validation rules. What do you think?
|
Thanks @jafranc for your feedback, I'll take them into account. |
|
@jafranc @rrsettgast the rule page is now separated in two and correctly linked in the indexes |
MelReyCG
added
ci: run CUDA builds
src/docs/sphinx/developerGuide/Contributing/CodingPractices.rst
Outdated
Show resolved
Hide resolved
| **Avoid magic values**: | ||
|
|
||
| - **arbitrary values should not be written more than once**, define constants, consider using or extending ``PhysicsConstants.hpp`` / ``Units.hpp``, | ||
| - **Prefer to let appear the calculus of constants** rather than writing its value directly without explaination (constexpr has no runtime cost). |
There was a problem hiding this comment.
I would be tempted to include a smooth transition and link to the next page CodeGeosFeatures.rst
Co-authored-by: Jacques Franc <49998870+jafranc@users.noreply.github.com>
MelReyCG
added
flag: no rebaseline
| - **Improves code safety,** preventing accidental modification for constant contexts, | ||
| - **Show code intention,** making code clearer. | ||
|
|
||
| Also, **mark methods const if the method is not designed to modify** the object state. |
There was a problem hiding this comment.
Any recommendations on mutable?
There was a problem hiding this comment.
Nice remark, I would feel like discouraging it but I see it is used at various locations (in favor of std::unique_ptrs).
There was a problem hiding this comment.
I'm adding a proposal:
Use of ``mutable``
------------------
``mutable`` **keyword usage is to avoid, excepted for specific patterns.**
Acceptable uses:
- **Thread synchronization primitives** (``std::mutex``, ``std::atomic`` for counters),
- **Caching** when it preserves logical constness, is thread-safe and clearly documented.
Avoid ``mutable`` for:
- Working around design issues (consider redesigning instead, i.e. splitting data structures),
- GPU-accessible data (creates host/device LvArray synchronization issues).
.. dropdown:: Why mutable is discouraged?
:icon: info
- **Breaks const methods contract:** Makes reasoning about code harder
- **Thread-safety confusion:** ``const`` methods may not be thread-safe
- **GPU incompatibility:** Can cause memory coherence issues with LvArray views
|
|
||
| - **Hoist Loop Invariants**, move computations that don't change during iterations outside the loop. | ||
| - When it does not critically affect the code architecture and clarity, **fuse multiple related kernels to reduce memory traffic and launch overhead** (i.e., statistics kernels process all physics field at once). | ||
| - **Optimize Memory Access for Cache and Coalescing**. Access memory sequentially and ensure coalesced access, especially on GPUs. |
There was a problem hiding this comment.
I think an example here would be helpful.
There was a problem hiding this comment.
I added a reference
| Wrapper Documentation (User-Oriented) | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| All data repository wrappers must be documented with ``setDescription()``. |
There was a problem hiding this comment.
I think we should encourage the specification of units where this makes sense.
| Code Coverage | ||
| ^^^^^^^^^^^^^ | ||
|
|
||
| **Code coverage should never decrease.** New code contributions must maintain or improve overall code coverage. |
There was a problem hiding this comment.
I think code coverage is based only on unit tests. Sometimes you ma add lines to fix a bug deep in a physics solver kernel that is only exercised by integrated tests.
There was a problem hiding this comment.
You're right, code cov should include integrated tests for this rule, I'll look how to do that.
There was a problem hiding this comment.
I added a remark in the meanwhile, and will proceed to do some tests in that way:
**Code coverage should never decrease.** New code contributions must maintain or improve overall code coverage.
Use Codecov to report untested code paths.
Currently, Codecov does not cover integrated tests execution but they should be taken into account, especially for minor physical kernel changes.
6 participants
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.
This PR is before #3914