Skip to content

Commit

Permalink
Fix merge conflict with develop
Browse files Browse the repository at this point in the history
  • Loading branch information
dbarrow257 committed Nov 20, 2024
2 parents 56df799 + 200f7c6 commit cbc4c9b
Show file tree
Hide file tree
Showing 99 changed files with 2,283 additions and 2,081 deletions.
88 changes: 73 additions & 15 deletions .github/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,16 @@ void Foo(){}
try
```cpp
/// @brief I like comments
/// @details anything longer than ~80 should go in a details
void Foo(){}
```
After making release or tag please
```bash
cd Doc
doxygen Doxyfile
```
to produce both html and latex file. To produce book like pdf file:
```bash
cd latex
pdflatex refman.tex
You can
```cpp
/// @param blarb this is parameter needed for function
/// @return returning some fancy number
int Fool(int blarb){}
```
Doxygen documentation will be updated after each commit to develop.
## Logger
MaCh3 is using spdlog logger see [here](https://github.com/gabime/spdlog/tree/master). And wraps it around MaCh3 names [here](https://github.com/mach3-software/MaCh3/blob/develop/manager/MaCh3Logger.h)
Expand Down Expand Up @@ -63,7 +61,7 @@ bool AsimovFit = false;
if(config[AsimovFit])
{
AsimovFit = config[AsimovFit].as<bool>;
AsimovFit = config[AsimovFit].as<bool>();
}
```
This can be replaced with:
Expand All @@ -75,17 +73,18 @@ bool AsimovFit = GetFromManager<bool>(config[AsimovFit], false);
Some fits require a lot of RAM. The easiest and fastest solution to reduce RAM
is to use `float` instead of `double`.

MaCh3 has a custom type defined as `_float_`, which is usually a `double`
MaCh3 has a custom type defined as `M3::float_t`, which is usually a `double`
unless the `_LOW_MEMORY_STRUCTS_` directive is defined at the compilation
level. When defined, `_float_` will be an actual `float`.
level. When defined, `M3::float_t` will be an actual `float`.

By using `_float_`, one can flexibly change between these types. During
By using `M3::float_t`, one can flexibly change between these types. During
development, it is advised to use these data types unless specific data
types are necessary due to desired precision, code safety, etc.

## Error handling
MaCh3 uses custom error handling implemented [here](https://github.com/mach3-software/MaCh3/blob/develop/manager/MaCh3Exception.h)
Instead of throw

Never ever ever bare throw. Always throw an exception, preferably one that subclasses one defined by the standard library in `<stdexcept>`.
```cpp
throw;
```
Expand All @@ -98,7 +97,66 @@ or
```cpp
throw MaCh3Exception(__FILE__ , __LINE__ );
```
This way we can ensure error messages are unified and user always get hints where in the code problem occurred. If current MaCh3Exception is not sufficient consider implementing new or expanding current exceptions in MaCh3Exception.h.
This way we can ensure error messages are unified and user always get hints where in the code problem occurred. If current `MaCh3Exception` is not sufficient consider implementing new or expanding current exceptions in MaCh3Exception.h.

## Compiler warning levels getting you down?

If you are trying to compile some new development and it is failing because of some innocuous warning that has been elevated to an error by the compiler flags, please don't just turn off the flags. A much better approach is to disable the diagnostic locally. This makes it easier to keep most of the code stringently checked, while giving you, the developer, the ability to stay in flow.
It also allows for later 'fixing' of these warnings, if they need to be fixed, to be done systematically by greping for the relevant directives.

The way to turn off diagnostics is, as below:

```c++
#pragma GCC diagnostic ignored "-Wfloat-conversion"
```

N.B. that clang also understands these directives, so don't panic that they have `GCC` in them.

This will disable that diagnostic for the rest of the compilation unit (usually a .cc file). Note that this means if you include these in headerfiles, they will disable diagnostics more widely, please try and disable the diagnostics over as little code as possible.

If a specific error is really getting you down and its showing up everywhere, the serious option is to disable it repo-wide by modifying the `MaCh3Warnings` interface target, defined in the top-level project [CMakeLists.txt](../CMakeLists.txt) like so:

```cmake
target_compile_options(MaCh3Warnings INTERFACE
# ...
-Wno-conversion
# ...
)
```

Please attempt more localised options before reaching for this, but sometimes this represents the best way to proceed with development without 'fixing' innocuous warnings.

The really serious option is to configure with: `-DMaCh3_WERROR_ENABLED=OFF`, which will disable the `-Werror` flag.

### An example

We got this compiler error:

```shell
/root/software/MaCh3/MaCh3_splitpr/splines/splineFDBase.cpp: In member function ‘virtual void splineFDBase::CalcSplineWeights()’:
/root/software/MaCh3/MaCh3_splitpr/splines/splineFDBase.cpp:349:35: error: useless cast to type ‘double’ [-Werror=useless-cast]
349 | weightvec_Monolith[iSpline] = double(weight);
| ^~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
```

for this code:

```c++
weightvec_Monolith[iSpline] = double(weight);
```

The compiler is right, that this is a useless cast, but `weight` can sometimes be a float, in which case we would get a conversion warning/error, so it seems like a no-win situation. We can 'save' the current diagnostics with `#pragma GCC diagnostic push`, disable the relevant one as above, and then revert to the saved set of diagnostics with `#pragma GCC diagnostic pop`.
Putting it all together might look like:

```c++
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wuseless-cast"
weightvec_Monolith[iSpline] = double(weight);
#pragma GCC diagnostic pop
```

This allows us to disable the diagnostic just for the relevant line.

## Formatting
To ensure a unified style in MaCh3 software you can use a clang-format file which has instructions about formatting code.
Expand Down
71 changes: 38 additions & 33 deletions .github/workflows/CDImage.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
---
# Update MaCh3 container image registry with newest updates
name: Image CD

Expand All @@ -24,6 +25,10 @@ jobs:
file: Doc/MaCh3DockerFiles/Alma9/Dockerfile
tag_latest: alma9latest
installoptions: -j4
- os: rocky9cuda
file: Doc/MaCh3DockerFiles/Rocky9/Dockerfile
tag_latest: rocky9cudalatest
installoptions: -j4
# - os: ubuntu22.04
# file: Doc/MaCh3DockerFiles/Ubuntu22.04/Dockerfile
# tag_latest: ubuntu22.04latest
Expand All @@ -36,40 +41,40 @@ jobs:
name: Image CD ${{ matrix.os }}

steps:
- name: Checkout repository
uses: actions/checkout@v4
- name: Checkout repository
uses: actions/checkout@v4

- name: Log in to GitHub Container Registry
run: echo "${{ secrets.GITHUB_TOKEN }}" | docker login ghcr.io -u ${{ github.actor }} --password-stdin
- name: Log in to GitHub Container Registry
run: echo "${{ secrets.GITHUB_TOKEN }}" | docker login ghcr.io -u ${{ github.actor }} --password-stdin

- name: Build Docker image
run: |
if [ "${{ github.ref_type }}" == 'tag' ]; then
docker build . \
--file ${{ matrix.file }} \
--tag ghcr.io/${{ github.repository_owner }}/mach3:${{ matrix.os }}${{ github.ref_name }} \
--build-arg MACH3_VERSION=${{ github.ref_name }} \
--build-arg INSTALL_OPTIONS="${{ matrix.installoptions }}"
else
docker build . \
--file ${{ matrix.file }} \
--tag ghcr.io/${{ github.repository_owner }}/mach3:${{ matrix.tag_latest }} \
--build-arg MACH3_VERSION=develop \
--build-arg INSTALL_OPTIONS="${{ matrix.installoptions }}"
fi
- name: Build Docker image
run: |
if [ "${{ github.ref_type }}" == "tag" ]; then
docker build . \
--file "${{ matrix.file }}" \
--tag "ghcr.io/${{ github.repository_owner }}/mach3:${{ matrix.os }}${{ github.ref_name }}" \
--build-arg MACH3_VERSION="${{ github.ref_name }}" \
--build-arg INSTALL_OPTIONS="${{ matrix.installoptions }}"
else
docker build . \
--file "${{ matrix.file }}" \
--tag "ghcr.io/${{ github.repository_owner }}/mach3:${{ matrix.tag_latest }}" \
--build-arg MACH3_VERSION="develop" \
--build-arg INSTALL_OPTIONS="${{ matrix.installoptions }}"
fi
- name: Push Docker image
run: |
if [ "${{ github.ref_type }}" == 'tag' ]; then
docker push ghcr.io/${{ github.repository_owner }}/mach3:${{ matrix.os }}${{ github.ref_name }}
else
docker push ghcr.io/${{ github.repository_owner }}/mach3:${{ matrix.tag_latest }}
fi
- name: Push Docker image
run: |
if [ "${{ github.ref_type }}" == "tag" ]; then
docker push "ghcr.io/${{ github.repository_owner }}/mach3:${{ matrix.os }}${{ github.ref_name }}"
else
docker push "ghcr.io/${{ github.repository_owner }}/mach3:${{ matrix.tag_latest }}"
fi
- name: Delete old images
uses: actions/delete-package-versions@v5
with:
package-name: 'mach3'
package-type: 'container'
min-versions-to-keep: 5
delete-only-untagged-versions: 'true'
- name: Delete old images
uses: actions/delete-package-versions@v5
with:
package-name: 'mach3'
package-type: 'container'
min-versions-to-keep: 5
delete-only-untagged-versions: 'true'
4 changes: 4 additions & 0 deletions .github/workflows/CIBuild.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ jobs:
file: Doc/MaCh3DockerFiles/Fedora32/Dockerfile
tag: fedora32latest
cmakeoptions: -DMaCh3_PYTHON_ENABLED=ON -DMaCh3_LOW_MEMORY_STRUCTS_ENABLED=ON
- os: Rocky9 CUDA
file: Doc/MaCh3DockerFiles/Rocky9/Dockerfile
tag: rocky9latest
cmakeoptions: -DMaCh3_PYTHON_ENABLED=ON

name: Build CI ${{ matrix.os }}

Expand Down
43 changes: 43 additions & 0 deletions .github/workflows/CIPythonValidations.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Performs unit and integration testing

name: Validations Python CI

# The events that trigger the workflow
on:
pull_request:
branches: [ develop ]

permissions:
contents: read
packages: write

jobs:
build:
runs-on: ubuntu-latest

container:
image: ghcr.io/mach3-software/mach3:alma9latest

steps:
- uses: actions/checkout@v4

- name: Get MaCh3 Validations
run: |
cd /opt/
git clone https://github.com/mach3-software/MaCh3Tutorial.git MaCh3Validations
- name: Install pytest
working-directory: /opt/MaCh3Validations
run: |
mkdir python-test-modules
pip install -r ./CIValidations/PythonTests/requirements.txt -t python-test-modules/
- name: Validations
working-directory: /opt/MaCh3Validations
run: |
source /opt/root/v6-26-10/bin/thisroot.sh
export PYTHONPATH=$PYTHONPATH:$PWD/python-test-modules/
export MACH3=$PWD
python3 -m pytest --config Inputs/ManagerTest.yaml CIValidations/PythonTests
8 changes: 4 additions & 4 deletions .github/workflows/CIValidations.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ jobs:
fail-fast: false # Prevents cancellation of remaining jobs if one fails
matrix:
include:
- name: Spline Validations
- name: Reweight Validations
test_1: ./CIValidations/SplineValidations
test_2: ./CIValidations/SamplePDFValidations
test_3: empty
test_3: ./CIValidations/NuOscillatorInterfaceValidations
test_4: empty
test_5: empty
test_6: empty
Expand All @@ -33,7 +33,7 @@ jobs:
- name: Covariance Validations
test_1: ./CIValidations/CovarianceValidations
test_2: ./CIValidations/MaCh3ModeValidations
test_3: empty
test_3: ./CIValidations/UnitTests/manager_tests
test_4: empty
test_5: empty
test_6: empty
Expand Down Expand Up @@ -75,7 +75,7 @@ jobs:
cd MaCh3Validations
mkdir build
cd build
cmake ../ -DMaCh3_Branch=${{ github.head_ref }}
cmake ../ -DMaCh3_Branch=${{ github.head_ref }} -DMaCh3Tutorial_UNITTESTS_ENABLED=TRUE
- name: Build MaCh3 Validations
run: |
Expand Down
13 changes: 7 additions & 6 deletions .github/workflows/Doxygen.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,19 +48,20 @@ jobs:
- run: sudo apt-get install -y perl

# Runs a single command using the runners shell
- name: Doxygen Action
uses: mattnotmitt/[email protected]
- uses: DenverCoder1/[email protected]
with:
doxyfile-path: './Doxyfile'
working-directory: ./Doc
github_token: ${{ secrets.GITHUB_TOKEN }}
folder: Doc/html
branch: gh-pages
config_file: Doc/Doxyfile

- name: Upload Doxygen Artifact
uses: actions/upload-artifact@v4
with:
retention-days: 1
name: DoxygenHTML
path: Doc/html

Sphinx:
# The type of runner that the job will run on
runs-on: ubuntu-latest
Expand Down Expand Up @@ -120,7 +121,7 @@ jobs:
name: DocumentationHTML
path: to-upload


Deploy:
runs-on: ubuntu-latest
needs: [Doxygen, Sphinx]
Expand Down
Loading

0 comments on commit cbc4c9b

Please sign in to comment.