-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding automated tests. #30
Conversation
476e9fa
to
09ebfe9
Compare
Okay, this exhibits all of the usual irritations with submodules, SSH keys and all that, so it's not the quick victory I was hoping for. |
b909f97
to
72c453d
Compare
This adds some automated tests for MAM4xx. NOTE: we need to open up the HAERO repo before this branch graduates to a PR.
1a283e1
to
2553717
Compare
Finally, we're getting somewhere. @pbosler , this workflow is doing what it needs to do. In particular, one of the single-precision builds is failing on a static assertion in @cameronrutherford , when we merge this PR, we'll have basic GitHub Actions CI for CPU testing. |
…Kohler polynomial requires double precision due to numerical ill-conditioning)
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That array bounds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has already been merged, but I was out of the office and was going through emails. I think I should create issues for these and we can address them in the future, but I wanted to also open threads for discussion before doing so.
-DMAM4XX_HAERO_DIR=`pwd`/haero_install \ | ||
-G "Unix Makefiles" | ||
|
||
# TODO: clang-format seems unstable under different versions. Not helpful! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I ran into this for ExaGO recently - we ended up requiring a specific version of clang-format for all developers.
This is quite annoying, but if you pin a version, apply a single formatting commit, and enforce it moving forward, you shouldn't run into it again...
For some reason I was also finding that clang-format would be reluctant to pick up the provided .clang-format
without forcing it to dump it's used configuration before running formatting checks. It seemed like a bug that I was running into with clang-format version 10.
@@ -1,8 +1,9 @@ | |||
#include "kohler_verification.hpp" | |||
#include "mam4_test_config.hpp" | |||
#ifdef HAERO_DOUBLE_PRECISION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an #ifdef
for the whole file? Would it be better to configure usage of this file at the CMake level instead?
@@ -1,6 +1,8 @@ | |||
#ifndef MAM4XX_KOHLER_VERIFICATION_HPP | |||
#define MAM4XX_KOHLER_VERIFICATION_HPP | |||
|
|||
#include <haero/haero.hpp> | |||
#ifdef HAERO_DOUBLE_PRECISION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar thought here - I think we should be able to handle this at the CMake level, not in the header file.
We should perhaps include this header conditionally in other files as well based on whether we build with double or single precision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with changing it to a CMake-handled inclusion. I wanted CI to get going quickly, and this was my first fast idea that worked.
cd build | ||
ctest -V | ||
|
||
# No install target (yet?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote all of ExaGO and HiOp's CMake install configurations, and would be happy to take a stab at helping here.
I have spent quite some time in the weeds of CMake, and it should make this relatively painless to configure.
EkatCreateUnitTest(mam4_kohler_unit_tests mam4_kohler_unit_tests.cpp | ||
LIBS mam4xx_tests ${HAERO_LIBRARIES}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should disable building and include of related tests into CMake - https://gitlab.pnnl.gov/exasgd/frameworks/exago/-/tree/master/buildsystem/cmake. We have developed some really helpful CMake macros that could be useful to enable this out of the box.
This adds some automated tests for MAM4xx.