Add more information to the contributing section especially laying out concerns for how to develop in singularity-eos and expectations for code review

CHANGELOG.md

@@ -5,45 +5,37 @@
 ### Fixed (Repair bugs, etc)
 
 ### Added (new features/APIs/variables/...)
+- [[PR209]](https://github.com/lanl/singularity-eos/pull/209) added more documentation around how to contribute to the project and also what a contributor can expect from the core team
 
 ### Changed (changing behavior/API/variables/...)
 
-### Fixed (not changing behavior/API/variables/...)
-
 ### Infrastructure (changes irrelevant to downstream codes)
 
 ### Removed (removing behavior/API/varaibles/...)
 
-### Added (modifiers with python bindings)
-
 ## Release 1.7.0
 Date: 12/14/2022
 
 ### Fixed (Repair bugs, etc)
 
 ### Added (new features/APIs/variables/...)
+- [[PR209]](https://github.com/lanl/singularity-eos/pull/209) added more documentation around how to contribute to the project and also what a contributor can expect from the core team
 
 ### Changed (changing behavior/API/variables/...)
 - [[PR192]](https://github.com/lanl/singularity-eos/pull/192) remove h5py and start using gold files
 
 ### Fixed (not changing behavior/API/variables/...)
+- [[PR198]](https://github.com/lanl/singularity-eos/pull/198) Use a more robust HDF5 handling that eliminates some edge breaks
+- [[PR181]](https://github.com/lanl/singularity-eos/pull/181) change from manual dependecy handling to using hdf5 interface targets
 
 ### Infrastructure (changes irrelevant to downstream codes)
 - [[PR206]](https://github.com/lanl/singularity-eos/pull/206) add another way to build sphinx docs
 
 ### Removed (removing behavior/API/varaibles/...)
 
-### Added (modifiers with python bindings)
-
 ## Release 1.6.2
 Date: 10/12/2022
 
-### Revert to standard `hdf5` handling
-- [[PR198]](https://github.com/lanl/singularity-eos/pull/198) Use a more robust HDF5 handling that eliminates some edge breaks
-
-### Updated `hdf5` handling in the build
-- [[PR181]](https://github.com/lanl/singularity-eos/pull/181) change from manual dependecy handling to using hdf5 interface targets
-
 ### Fixed (Repair bugs, etc)
 - [[PR183]](https://github.com/lanl/singularity-eos/pull/183) fortify cmake export config to always have interface targets of dependencies that need them
 - [[PR174]](https://github.com/lanl/singularity-eos/pull/174) fix build configuration when closures are disabled

doc/sphinx/src/contributing.rst

@@ -37,6 +37,81 @@ internal tests will not be run automatically. So when the code is
 ready for merge, you must ask a project maintainer to trigger the
 remaining tests for you.
 
+Expectations for code review
+-----------------------------
+
+From the perspective of the contributor
+````````````````````````````````````````
+
+Code review is an integral part of the development process
+for ``singularity-eos``. You can expect at least one, perhaps many,
+core developers to read your code and offer suggestions.
+You should treat this much like scientific or academic peer review.
+You should listen to suggestions but also feel entitled to push back
+if you believe the suggestions or comments are incorrect or
+are requesting too much effort.
+
+Reviewers may offer conflicting advice, if this is the case, it's an
+opportunity to open a discussion and communally arrive at a good
+approach. You should feel empowered to argue for which of the
+conflicting solutions you prefer or to suggest a compromise. If you
+don't feel strongly, that's fine too, but it's best to say so to keep
+the lines of communication open.
+
+Big contributions may be difficult to review in one piece and you may
+be requested to split your pull request into two or more separate
+contributions. You may also receive many "nitpicky" comments about
+code style or structure. These comments help keep a broad codebase,
+with many contributors uniform in style and maintainable with
+consistent expectations accross the code base. While there is no
+formal style guide for now, the regular contributors have a sense for
+the broad style of the project. You should take these stylistic and
+"nitpicky" suggestions seriously, but you should also feel free to
+push back.
+
+As with any creative endeavor, we put a lot of ourselves into our
+code. It can be painful to receive criticism on your contribution and
+easy to take it personally. While you should resist the urge to take
+offense, it is also partly code reviewer's responsiblity to create a
+constructive environment, as discussed below.
+
+Expectations of code reviewers
+````````````````````````````````
+
+A good code review builds a contribution up, rather than tearing it
+down. Here are a few rules to keep code reviews constructive and
+congenial:
+
+* You should take the time needed to review a contribution and offer
+  meaningful advice. Unless a contribution is very small, limit
+  the times you simply click "approve" with a "looks good to me."
+
+* You should keep your comments constructive. For example, rather than
+  saying "this pattern is bad," try saying "at this point, you may
+  want to try this other pattern."
+
+* Avoid language that can be misconstrued, even if it's common
+  notation in the commnunity. For example, avoid phrases like "code
+  smell."
+
+* Explain why you make a suggestion. In addition to saying "try X
+  instead of Y" explain why you like pattern X more than pattern Y.
+
+* A contributor may push back on your suggestion. Be open to the
+  possibility that you're either asking too much or are incorrect in
+  this instance. Code review is an opportunity for everyone to learn.
+
+* Don't just highlight what you don't like. Also highlight the parts
+  of the pull request you do like and thank the contributor for their
+  effort.
+
+General principle for everyone
+```````````````````````````````
+
+It's hard to convey tone in text correspondance. Try to read what
+others write favorably and try to write in such a way that your tone
+can't be mis-interpreted as malicious.
+
 Interwoven Dependencies
 ------------------------
 
@@ -50,6 +125,118 @@ and we'll engage with you to figure out how to proceed.
 Notes for Contribuors
 ---------------------------------------
 
+Some notes on style and code architecture
+``````````````````````````````````````````
+
+* ``singularity-eos`` is primarily designed to provide needed equation
+  of state functionality to continuum dynamics codes. It isn't
+  supposed to provide the most accurate or complete picture of thermal
+  or statistical physics. As such the project tries to limit
+  capabilities to this scope.
+
+* A major influence on code style and architecture is the
+  `ten rules for developing safety-critical code`_, by Gerard Holzmann.
+  Safety critical code is code that exists in a context where failure
+  implies serious harm. A flight controler on an airplane or
+  spacecraft or the microcontroller in a car are examples of
+  safety-critical contexts. ``singularity-eos`` is not safety-critical
+  but many of the coding habits advocated for by Holzmann produce
+  long-lived, easy to understand, easy to parse, and easy to maintain code.
+  And we take many of the rules to heart. Here are a few that are most
+  relevant to ``singularity-eos``. They have been adapted slightly to 
+  our context.
+
+    #. Avoid complex flow constructs such as gotos.
+
+    #. All loops must have fixed bounds. This prevents runaway
+       code. (Note this implies that as a general rule, one should use
+       ``for`` loops, not ``while`` loops. It also implies one should
+       keep recursion to a minimum.)
+
+    #. Heap memory allocation should only be performed at
+       initialization. Heap memory de-allocation should only be
+       performed at cleanup.
+
+    #. Restrict the length of functions to a single printed page.
+
+    #. Restrict the scope of data to the smallest possible.
+
+    #. Use the preprocessor sparingly.
+
+    #. Limit pointer use to a single dereference. Avoid pointers of
+       pointers when possible.
+
+    #. Be compiler warning aware. Try to address compiler warnings as
+       they come up.
+
+.. _ten rules for developing safety-critical code: http://web.eecs.umich.edu/~imarkov/10rules.pdf
+
+* ``singularity-eos`` is a modern C++ code
+  and both standard template library capabilities and template
+  metaprogramming are leveraged frequently. This can sometimes make
+  parsing the code difficult. If you see something you don't
+  understand, ask. It may be it can be refactored to be more simple or
+  better documented.
+
+Performance portability concerns
+`````````````````````````````````
+
+``singularity-eos`` is performance portable, meaning it is designed to
+run not only on CPUs, but GPUs from a variety of manufacturers,
+powered by a variety of device-side development tools such as Cuda,
+OpenMP, and OpenACC. This implies several constraints on code
+style. Here we briefly discuss a few things one should be aware of.
+
+* **``ports-of-call`` and portability decorators:** Functions that
+  should be run on device needs to be decorated with one of the
+  following macros: ``PORTABLE_FUNCTION``,
+  ``PORTABLE_INLINE_FUNCTION``,
+  ``PORTABLE_FORCEINLINE_FUNCTION``. These macros are imported from
+  the `ports-of-call`_ library and resolve to the appropriate
+  decorations for a given device-side backend such as cuda so the code
+  compiles correctly. Code that doesn't need to run on device does not
+  need these decorations.
+
+* **Relocatable device code:** It is common in C++ to split code
+  between a header file and an implementation file. Functionality that
+  is to be called from within loops run on device should not be split
+  in this way. Not all accelerator languages support this and the ones
+  that do take a performance hit. Instead implement that functionality
+  only in a header file and decorate it with
+  ``PORTABLE_INLINE_FUNCTION`` or ``PORTABLE_FORCEINLINE_FUNCTION``.
+
+* **Host and device pointers:** Usually accelerators have different
+  memory spaces than the CPU they are attached to. So you need to be
+  aware that data needs to be copied to an accelerator device to be
+  used. If it is not properly copied, the code will likely crash with
+  a segfault. In general scalar data such as a single variable (e.g.,
+  ``int x``) can be easily and automatically copied to device and you
+  don't need to worry about managing it. Arrays and pointers, however,
+  are a different story. If you create an array or point to some
+  memory on CPU, then you are pointing to a location in memory on your
+  CPU. If you try to access it from your accelerator, your code will
+  not behave properly. You need to manually copy data from host to
+  device in this case. The libraries `ports-of-call`_ and `spiner`_
+  offer some functionality for managing arrays on device.
+
+* **Shallow copies:** As a general rule, large
+  amount of data stored within an ``EOS`` object should have
+  "reference-semantics." This means that if you copy an EOS object, it
+  should always be a shallow copy, not a deep copy, unless a deep copy
+  is explicitly requested. This is for performance reasons and also to
+  simplify the managment of data on device.
+
+* **Real:** The ``Real`` datatype is either a single precision or
+  double precision floating point number, depending on how
+  `ports-of-call`_ is configured. For most floating point numbers use
+  the ``Real`` type. However, be conscious that sometimes you will
+  specifically need a single or double precision number, in which case
+  you should specify the type as built into the language.
+
+.. _ports-of-call: https://lanl.github.io/ports-of-call/main/index.html
+
+.. _spiner: https://lanl.github.io/spiner/main/index.html
+
 The CRTP slass structure and static polymorphism
 ````````````````````````````````````````````````
 
@@ -85,8 +272,6 @@ lookup in the specific EOS. However, this means that the base class needs to
 have knowledge of which class is being derived from it in order to call the
 correct EOS implementation.
 
-
-
 The standard solution to this problem would be "run-time inheritence,"
 where type deduction is performed at run-time. While this is possible
 on GPU, it becomes cumbersome, as the user must be very explicit about