CI: add linting #618

GPMueller · 2022-05-01T13:36:12Z

add C++ format linting and clang-tidy reporting for the core library
add C++ format linting and clang-tidy reporting for the ImGUI
add Python format and quality linting for the core library wrappers and tests
add Python format and quality linting for the ui-python folder

codecov · 2022-05-01T13:46:44Z

Codecov Report

Merging #618 (24c4cc3) into develop (f19764f) will increase coverage by 0.31%.
The diff coverage is 54.94%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #618      +/-   ##
===========================================
+ Coverage    48.16%   48.48%   +0.31%     
===========================================
  Files           74       75       +1     
  Lines        11968    12015      +47     
===========================================
+ Hits          5764     5825      +61     
+ Misses        6204     6190      -14

coveralls · 2022-05-01T14:11:01Z

Coverage: 80.833% (+0.1%) from 80.705% when pulling 24c4cc3 on feature-ci-linting into f19764f on develop.

Should hopefully build with clang-15. Needed to replace uses of `REQUIRE( ... == Approx( ... ) )` with `REQUIRE_THAT( ... WithinAbs( ... ) )`.

GPMueller · 2023-02-08T18:08:48Z

I've managed to fix the CI, except:

Depondt and SIB solvers don't achieve the expected precision when using OpenMP (the Depondt solver can be fixed with c982a34)
test_anisotropy.cpp contains a test comparing energies from Energy and Gradient_and_Energy, which only succeeds when the precision is lowered to 1e-4

GPMueller · 2023-02-09T14:37:16Z

core/src/engine/Hamiltonian_Heisenberg.cu

@@ -1,38 +1,37 @@
 #ifdef SPIRIT_USE_CUDA


@MSallermann can you take a closer look at this file, and maybe also execute tests using CUDA on this branch?

These changes are quite difficult to review and make sure they're OK just from looking at them...

I think it's OK. I can build and run the Code with CUDA. Many of the unit tests fail though, because the requested precision is too high. I think it would be best to use different epsilons for double and single precision builds

This test trips up the Heun and SIB solvers when requiring higher precision. This is an issue in their implementations and should really be fixed.

- update checkout action to v3, setup-python action to v4 - add a Windows OpenMP build using LLVM - add an Ubuntu 22.04 CUDA build On Ubuntu 22.04, we get gcc 11.3 and nvidia-cuda-toolkit 11.5, which are not compatible, producing compilation errors. Therefore, a downgrade of gcc to version 10 is needed. Note: had to change `ChooseCompiler.cmake` to enable user-chosen compilers via `USER_COMPILER_CXX` etc.

GPMueller · 2023-02-11T23:12:29Z

@MSallermann I believe this PR is now ready

MSallermann · 2023-02-15T15:09:54Z

I've managed to fix the CI, except:

* Depondt and SIB solvers don't achieve the expected precision when using OpenMP (the Depondt solver can be fixed with [](https://github.com/spirit-code/spirit/commit/c982a34c604aa763bb615123be6fb442738a089c)

* `test_anisotropy.cpp` contains a test comparing energies from `Energy` and `Gradient_and_Energy`, which only succeeds when the precision is lowered to `1e-4`

I have pushed another commit to the suzuki_trotter branch 04e6b89. With this, and the commit you mentioned, I can set epsilon for the Larmor precession test to 1e-9 (as opposed to 1e-6 at the moment) and all solvers pass.
The problem with the last assert in test_anisotropy is that a float and a double are compared and the float truncates the last digits

GPMueller · 2023-02-16T13:37:39Z

core/test/test_anisotropy.cpp

-    REQUIRE( Approx( normal[0] ) == 0 );
-    REQUIRE( Approx( normal[1] ) == 0 );
-    REQUIRE( Approx( normal[2] ) == 1 );
+    REQUIRE_THAT( magnitude, WithinAbs( init_magnitude, 1e-12 ) );


Need to set the 1e-12 epsilon value dependent on the precision, otherwise tests will fail when using single precision (float)

GPMueller · 2023-02-16T13:37:50Z

core/test/test_manifoldmath.cpp

    int N       = 10000;
    int N_check = std::min( 100, N );
    vectorfield v1( N, Vector3{ 0.0, 0.0, 1.0 } );
    vectorfield v2( N, Vector3{ 1.0, 1.0, 1.0 } );

-    REQUIRE( Engine::Vectormath::dot( v1, v2 ) == Approx( N ) );
+    REQUIRE_THAT( Engine::Vectormath::dot( v1, v2 ), WithinAbs( N, 1e-12 ) );


Need to set the 1e-12 epsilon value dependent on the precision, otherwise tests will fail when using single precision (float)

GPMueller · 2023-02-16T13:38:24Z

core/test/test_vectormath.cpp

-        REQUIRE( m[0] == Approx( 0 ) );
-        REQUIRE( m[1] == Approx( 0 ) );
-        REQUIRE( m[2] == Approx( mu_s ) );
+        REQUIRE_THAT( m[0], WithinAbs( 0, 1e-12 ) );


Need to set the 1e-12 epsilon value dependent on the precision, otherwise tests will fail when using single precision (float)

GPMueller · 2023-02-16T13:40:07Z

core/test/test_solvers.cpp

    // Input file
    auto inputfile = "core/test/input/solvers.cfg";

    // State
    auto state = std::shared_ptr<State>( State_Setup( inputfile ), State_Delete );

    // Reduce precision if float accuracy
-    double epsilon_apprx = 1e-5;
+    float epsilon_apprx = 1e-5;


should this epsilon be a scalar value?

MSallermann · 2023-02-15T15:32:29Z

core/test/test_anisotropy.cpp

    state->active_image->hamiltonian->Gradient_and_Energy( spins, grad, energy3 );

    energy1 = state->active_image->hamiltonian->Energy( spins );
-    REQUIRE( Approx( energy3 ) == energy1 );
+    REQUIRE_THAT( energy3, WithinAbs( energy1, 1e-4 ) ); // TODO: why the low precision?


The variable energy1 is a float and being compared to a double (if not using CUDA). Maybe it would be better to not re-use energy1 and just declare a new scalar energy4. Then the precision of the test can be set much higher.

MSallermann · 2023-02-15T15:48:03Z

core/src/engine/Hamiltonian_Heisenberg.cu

@@ -1,38 +1,37 @@
 #ifdef SPIRIT_USE_CUDA


I think it's OK. I can build and run the Code with CUDA. Many of the unit tests fail though, because the requested precision is too high. I think it would be best to use different epsilons for double and single precision builds

MSallermann · 2023-02-15T16:07:04Z

core/test/test_manifoldmath.cpp

-            == Approx( Engine::Manifoldmath::norm( v1 ) * Engine::Manifoldmath::norm( v2 ) ) );
+        REQUIRE_THAT(
+            Engine::Vectormath::dot( v1, v2 ),
+            !WithinAbs( Engine::Manifoldmath::norm( v1 ) * Engine::Manifoldmath::norm( v2 ), 1e-3 ) );


Is there a reason why the requested precision is so much lower here?

because there's a not there, we want to be sufficiently far away from the given value

GPMueller added 5 commits May 1, 2022 13:36

CI: add lint step for core and ui-imgui

7e4f0f6

Fix formatting of CI-linted files

0831dc3

CI: generate clang-tidy report to log

ccaa6be

CI: add Python linting

c2dc123

Reformatted Python code using black

97c5842

GPMueller added core ui-cpp ui-python CI core-python-api labels May 1, 2022

GPMueller requested a review from MSallermann May 1, 2022 13:36

Core: improve Python import statements

bd79ff0

GPMueller force-pushed the feature-ci-linting branch from a159ac0 to bd79ff0 Compare May 1, 2022 13:49

Merge branch 'develop' into feature-ci-linting

4c19c99

GPMueller force-pushed the feature-ci-linting branch from 04447dd to 66f24f5 Compare February 7, 2023 22:38

GPMueller added 3 commits February 7, 2023 23:56

CI: update clang in lint job to 15

0bbbf5c

Core: fix bad merge in CUDA Heisenberg Hamiltonian

12419fa

Formatting update

3285003

GPMueller force-pushed the feature-ci-linting branch 3 times, most recently from e1553d7 to 8c25c98 Compare February 8, 2023 13:21

Update catch to v2.13.6

d2a78d1

Should hopefully build with clang-15. Needed to replace uses of `REQUIRE( ... == Approx( ... ) )` with `REQUIRE_THAT( ... WithinAbs( ... ) )`.

GPMueller force-pushed the feature-ci-linting branch from 8c25c98 to d2a78d1 Compare February 8, 2023 15:42

GPMueller commented Feb 9, 2023

View reviewed changes

Core: decrease precision of Lamor precession test

53f3a0a

This test trips up the Heun and SIB solvers when requiring higher precision. This is an issue in their implementations and should really be fixed.

GPMueller force-pushed the feature-ci-linting branch 2 times, most recently from 4a4413d to b8032d1 Compare February 11, 2023 13:22

GPMueller force-pushed the feature-ci-linting branch 21 times, most recently from ec306f7 to 4cba9a3 Compare February 11, 2023 22:50

GPMueller force-pushed the feature-ci-linting branch from 4cba9a3 to 24c4cc3 Compare February 11, 2023 23:06

GPMueller commented Feb 16, 2023

View reviewed changes

GPMueller mentioned this pull request Feb 16, 2023

Cleanup unit tests #628

Merged

4 tasks

MSallermann reviewed Feb 16, 2023

View reviewed changes

MSallermann merged commit b5fac37 into develop Feb 16, 2023

GPMueller deleted the feature-ci-linting branch February 16, 2023 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: add linting #618

CI: add linting #618

GPMueller commented May 1, 2022

codecov bot commented May 1, 2022 •

edited

Loading

coveralls commented May 1, 2022 •

edited

Loading

GPMueller commented Feb 8, 2023 •

edited

Loading

GPMueller Feb 9, 2023

MSallermann Feb 15, 2023

GPMueller commented Feb 11, 2023

MSallermann commented Feb 15, 2023 •

edited

Loading

GPMueller Feb 16, 2023

GPMueller Feb 16, 2023

GPMueller Feb 16, 2023

GPMueller Feb 16, 2023

MSallermann Feb 15, 2023

MSallermann Feb 15, 2023

MSallermann Feb 15, 2023

GPMueller Feb 16, 2023

CI: add linting #618

CI: add linting #618

Conversation

GPMueller commented May 1, 2022

codecov bot commented May 1, 2022 • edited Loading

Codecov Report

coveralls commented May 1, 2022 • edited Loading

GPMueller commented Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GPMueller commented Feb 11, 2023

MSallermann commented Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 1, 2022 •

edited

Loading

coveralls commented May 1, 2022 •

edited

Loading

GPMueller commented Feb 8, 2023 •

edited

Loading

MSallermann commented Feb 15, 2023 •

edited

Loading