spirit-code / spirit

Atomistic Spin Simulation Framework

http://spirit-code.github.io

MIT License

120 stars 53 forks source link

Cleanup unit tests #628

Closed GPMueller closed 1 year ago

GPMueller commented 1 year ago

Cleaning up some untidy code and try to make tests more readable and easier to understand.

TODO

[x] clean up code
[x] add a single-precision, CPU (scalar == float) build to the CI tests
[x] fix the types of the epsilon and energy values (should fix anisotropy test precision) see https://github.com/spirit-code/spirit/pull/618#issuecomment-1431521441 and https://github.com/spirit-code/spirit/pull/618#discussion_r1108489205 and https://github.com/spirit-code/spirit/pull/618#discussion_r1107283136
[x] fix setting of precision constants / epsilons for single-precision see https://github.com/spirit-code/spirit/pull/618#issuecomment-1431521441

GPMueller commented 1 year ago

Note: the new API input checkers are proving quite useful in debugging test code as well:

2023-02-13 13:19:51  [ ERROR ] [API ] [01] [01]  -----------------------------------------------------
2023-02-13 13:19:51  [ ERROR ] [API ] [01] [01]  Caught exception in API function 'Simulation_LLG_Start' (at /home/runner/work/spirit/spirit/core/src/Spirit/Simulation.cpp:218)
                                                 Exception backtrace:
2023-02-13 13:19:51  [ ERROR ] [API ] [--] [--]  /home/runner/work/spirit/spirit/core/include/data/State.hpp:73 in function 'throw_if_nullptr':
                                                 Got passed a null pointer for 'info'
2023-02-13 13:19:51  [ ERROR ] [API ] [01] [01]  -----------------------------------------------------

codecov[bot] commented 1 year ago

Codecov Report

Merging #628 (ff41d9a) into develop (24c4cc3) will increase coverage by 0.56%. The diff coverage is 47.01%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #628 +/- ## =========================================== + Coverage 48.48% 49.04% +0.56% =========================================== Files 75 75 Lines 12015 12070 +55 =========================================== + Hits 5825 5920 +95 + Misses 6190 6150 -40 ```

coveralls commented 1 year ago

Coverage: 80.845% (+0.01%) from 80.833% when pulling ff41d9a2cecedbd3aa65c0ff8ded6abaaa9c740a on cleanup-unit-tests into b5fac37184300596f46ebff18a2f8e71da4755b0 on develop.

GPMueller commented 1 year ago

It seems that mixing single and double precision in the unit tests is a bad idea! The results are very tricky to predict correctly and it would be far preferable to fix the API to provide adaptible precision using the scalar alias, see issue #477. The only drawback of using the scalar alias would be that the web-UI that goes through the C API would have to be built with single precision, because the API needs to be single precision.

Not totally sure yet, but is looks like this PR might be a good place to do this work and fix issue #477.

GPMueller commented 1 year ago

This PR now fixes issue #477. The UI code is now somewhat inconsistent in terms of using float vs double vs scalar (for both the ImGui and Qt UIs), but I believe it's good enough.

@MSallermann this significantly breaks API compatibility, so if you'd prefer I can pull out the API changes into a separate PR, so that a new release can be made before breaking the API and bumping the major version number. On the other hand, while a quick diff between develop and master didn't show notable API breakage, we did already upgrade to C++14 since the last release, which IMO warrants a new major version anyways).