sxs-collaboration/spectre

SpECTRE is a code for multi-scale, multi-physics problems in astrophysics and gravitational physics.

spectre's Issues

Add operator/ for double/DataVector

It is not currently possible to write x = constant/y; for DataVectors x,y.

If we want to write intuitive expression templates for terms like the above, we should implement DataVector::operator/(const double&, const DataVector&) and the associated low-level infrastructure.

Note: we are working on a wrapper for x = DataVector(y.size(), constant)/y that will make the creation of a dummy numerator more generic and easy to use. The wrapper will provide a sufficient workaround, making this a low-priority issue. However, having an operator/ would still be cleaner and more intuitive.

Options: specify overall help-string near OptionsList

The per-option help-text is specified as part of the OptionsList, whereas the overall help-text is specified when the options are parsed. It would be nice if all help-text is given in one part of the .?pp file, i.e. all in the OptionList.

`atan2` function for DataVector

Currently the atan2 function does not work for DataVectors. This will ultimately need to be added to Blaze, but we can do it for now and then submit a PR to Blaze. Intel's SVML IS supports atan2 directly, see here.

Performing math on a Datavector and assigning back into it does not work.

The following code outputs 3 DataVectors full of zeroes. This isn't what one would expect to happen writing this code so I'm gonna say it's a bug.

DataVector x(4, 4.), y(4, 4.), z(4, 4.);

x = sqrt(x);
y = sin(y);
z = abs(z);
Parallel::printf("%s\n", x);
Parallel::printf("%s\n", y);
Parallel::printf("%s\n", z);

Can't compile -a/b with DataVectors

See this test case:

TEST_CASE("NegDiv", "[Unit][NegDiv]") {
  const DataVector ones(3, 1.0);
  const DataVector twos(3, 2.0);
  DataVector dummy;
  dummy = ones/twos;
  dummy = -(ones/twos);
  dummy = (twos-ones)/twos;
  // fail to compile:
  //dummy = -ones/twos;
  //dummy = -(twos-ones)/twos;
}

Inconsistent warnings between machines

As discussed a few days ago, we expected sign comparison warnings from PR #127 but didn't see any on Travis. It turns out I do see these warnings on my machine (with either gcc-5.4.0 or gcc-6.3.0), but, in addition to Travis, @osheamonn does not see them (6.3.0).

So something is causing inconsistencies, but I don't know what.

Add arithmetic operators for Point's

It's come up multiple times now that having operator+, operator-, and operator*, operator/ for scalar multiplication would be useful to have for Points. The current plan is for this to be implemented with 'TensorExpressions' but I'm unclear on what the time frame for this is. Also since Point is rarely, if ever, used in expressions as complicated as other tensors (i.e. probably not going to be contracted with higher rank tensors) we could simply implement these operators and not rely on the TensorExpressions. We'd like to have them now for DomainHelpers, numerical derivatives in TestHelpers, and the transfinite blending that Gabe is working on.

I've assigned myself to this since adding the simple operators shouldn't be difficult if this seems like a reasonable idea.

Document variable-step Adams-Bashforth internals

I was supposed to do this during PR #103, but I forgot.

Filesystem test issues

I noticed (due to running tests in parallel) that Unit.Utilities.FileSystem.create_and_rm_empty_directory checks for existence of the wrong file. I was going to just fix that, but then I saw that the create_directory tests don't actually check that the directories have been created in many cases. As this is getting non-trivial I'm just filing a bug instead of trying to provide a fix right now.

Add a math test for reference DataVectors

We have a few tests that check that DataVectors and reference DataVectors behave the same way, for instance,

Unit.Serialization.DataVector
Unit.Serialization.DataVector_Ref

We should add a test Unit.DataStructures.DataVector.Math_Ref to ensure that reference DataVectors behave correctly as arithmetic types.

Historical aside: in the previous repository, this check was (partially) handled within the Determinant test, but it really belongs as its own unit test.

document git-hooks

Add a few lines of documentation explaining that the git-pre-commit hooks are being set by cmake, and point to where this is done.

This could go into a README.md file inside hooks/

Also, change the message printed by the clang-pre-commit-hook. I suggest the following

Your commit does not follow the clang-format adopted by SpECTRE. Please ensure you are formatting your C++ files before committing them. Many editors/ides have commands to clang-format before or while saving. Alternatively, you can first save all modified files, and then run 'git clang-format -f'.

Perhaps even better, put that info on a documentation-page. Then shorten the error message to

Your commit does not follow the clang-format adopted by SpECTRE. Please ensure C++ files are clang-formatted before committing them. See for details.


should test pup functions of embedding maps

This can't be done until we port the make_items_pupable function is ported over

Have Travis run clang-tidy on changed and added cpp files

It would be very helpful to have clang-tidy run on all cpp files that are added or changed in a PR. The reason we only want to run on the diff is that clang-tidy takes longer to run than an actual compile. My concern is that we will run out of build time much earlier than we would normally if we run clang-tidy on the entire code every time. The best way to set up clang-tidy is likely to add a new Travis build target.

We might want to set up a Jenkins server to run clang-tidy on the entire code base once a day on a machine we control, but this is a separate issue.

Add not_null and switch to using pointers instead non-const refs

Google style guide suggests using pointers instead of non-const & so it's obvious from the call site that the argument may be modified. However, a pointer may be null and this can cause problems when dereferencing the pointer, and so using not_null from the GSL will be important.

This was discussed with @wthrowe @kidder @fmahebert @markscheel

Assertion when move assigning defaulted Variables

This

Variables<typelist<VariablesTestTags_detail::vector>> vars;
vars = Variables<typelist<VariablesTestTags_detail::vector>>{};

(using definitions from Test_Variables.cpp) aborts with

############ ASSERT FAILED ############
Line: 352 of file '/home/wthrowe/spectre/src/DataStructures/Variables.hpp'
Condition: size_ > (variable_offset + TagToAdd::type::size() - 1) * number_of_grid_points_
This ASSERT is typically triggered because a Variables class was default constructed. The only reason the Variables class has a default constructor is because Charm++ uses it, you are not supposed to use it otherwise.
#######################################

This operation occurs when you attempt to deserialize a tuple containing a Variables.

Support charm 6.8

Our 6.7 patch doesn't apply to 6.8, so at the least we'll need to update that. Hopefully that's all we'll need to do.

Test Labels for Definite Integral Are Bad

The test labels for Definite Integral should be "[Numerical][Spectral][Unit]" like the other spectral tests. @gsb76 this is your task to correct.

Add to code review guidelines variables in macro names

Should make variable names in macros be such that they will not shadow other variable names, eg avoid_name_collisions_PARSE_ERROR

Test GCC5 and GCC7 builds with TravisCI

I've set up a branch with Dockerfile that has GCC5 and GCC7 installed. However, because with Charm++6.7.1 and 6.8.0 there is no way to easily control the compiler version, I have not taken this further yet. The branch is here.

reduce clang-tidy noise

See https://travis-ci.org/sxs-collaboration/spectre/jobs/257403233

Reduce compile times

This issue will track thoughts, experiments and results of optimizing compile times. In particular, tests of explicit instantiations, header file dependencies, pre-compiled headers, and using CMake to generate cpp files for explicit instantiations.

Fix clang-tidy issues

A list of all (or at least nearly all) clang-tidy issues are available at https://travis-ci.org/sxs-collaboration/spectre/builds/261668157. Please check for portions of the code you are responsible for, and also run over the cpp files related to the code that is warned about in the tests. Thanks!

Compilatoin failure with gcc 5.4.0

/home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:83:1: error: non-constant condition for static assertion
 static_assert(array_equal(array1, array1_copy), "Failed testing array_equal");
 ^
In file included from /home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:6:0:
/home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:83:26:   in constexpr expansion of ‘array_equal<double, double, 3ul>({anonymous}::array1, {anonymous}::array1_copy, 0ul)’
/home/wthrowe/spectre/src/Utilities/ConstantExpressions.hpp:216:29: error: ‘constexpr const typename Cont::value_type& gsl::at(const Cont&, Size) [with Cont = std::array<double, 3ul>; Size = long unsigned int; typename Cont::value_type = double]’ called in a constant expression
   return i < size ? (gsl::at(lhs, i) == gsl::at(rhs, i) and
                             ^
In file included from /home/wthrowe/spectre/src/Utilities/ConstantExpressions.hpp:12:0,
                 from /home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:6:
/home/wthrowe/spectre/src/Utilities/Gsl.hpp:119:66: note: ‘constexpr const typename Cont::value_type& gsl::at(const Cont&, Size) [with Cont = std::array<double, 3ul>; Size = long unsigned int; typename Cont::value_type = double]’ is not usable as a constexpr function because:
 SPECTRE_ALWAYS_INLINE constexpr const typename Cont::value_type& at(
                                                                  ^
/home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:84:1: error: non-constant condition for static assertion
 static_assert(not array_equal(array1, std::array<double, 3>{{2.2, 3, 4}}),
 ^
In file included from /home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:6:0:
/home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:84:30:   in constexpr expansion of ‘array_equal<double, double, 3ul>({anonymous}::array1, std::array<double, 3ul>{std::__array_traits<double, 3ul>::_Type{2.2000000000000002e+0, 3.0e+0, 4.0e+0}}, 0ul)’
/home/wthrowe/spectre/src/Utilities/ConstantExpressions.hpp:216:29: error: ‘constexpr const typename Cont::value_type& gsl::at(const Cont&, Size) [with Cont = std::array<double, 3ul>; Size = long unsigned int; typename Cont::value_type = double]’ called in a constant expression
   return i < size ? (gsl::at(lhs, i) == gsl::at(rhs, i) and
                             ^
/home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:86:1: error: non-constant condition for static assertion
 static_assert(array_equal(replace_at<1>(array1, 5.0),
 ^
In file included from /home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:6:0:
/home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:86:52:   in constexpr expansion of ‘replace_at<1ul, double, 3ul>({anonymous}::array1, 5.0e+0)’
/home/wthrowe/spectre/src/Utilities/ConstantExpressions.hpp:204:54:   in constexpr expansion of ‘ConstantExpression_detail::replace_at_helper<double, 3ul, {0ul}, {0ul}>((* & arr), (*(const double*)std::forward<double>(value)), 2ul, (std::make_integer_sequence<long unsigned int, 1ul>{}, std::make_integer_sequence<long unsigned int, 1ul>()), (std::make_integer_sequence<long unsigned int, 1ul>{}, std::make_integer_sequence<long unsigned int, 1ul>()))’
/home/wthrowe/spectre/src/Utilities/ConstantExpressions.hpp:195:33: error: ‘constexpr const typename Cont::value_type& gsl::at(const Cont&, Size) [with Cont = std::array<double, 3ul>; Size = long unsigned int; typename Cont::value_type = double]’ called in a constant expression
       {arr[I]..., value, gsl::at(arr, i + J)...}};
                                 ^
/home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:91:1: error: non-constant condition for static assertion
 static_assert(
 ^
In file included from /home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:6:0:
/home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:92:16:   in constexpr expansion of ‘array_equal<long unsigned int, long unsigned int, 3ul>(std::array<long unsigned int, 3ul>{std::__array_traits<long unsigned int, 3ul>::_Type{1ul, 2ul, 5ul}}, make_array_from_list<brigand::list<brigand::integral_constant<long unsigned int, 1ul>, brigand::integral_constant<long unsigned int, 2ul>, brigand::integral_constant<long unsigned int, 5ul> >, 0u>(), 0ul)’
/home/wthrowe/spectre/src/Utilities/ConstantExpressions.hpp:216:29: error: ‘constexpr const typename Cont::value_type& gsl::at(const Cont&, Size) [with Cont = std::array<long unsigned int, 3ul>; Size = long unsigned int; typename Cont::value_type = long unsigned int]’ called in a constant expression
   return i < size ? (gsl::at(lhs, i) == gsl::at(rhs, i) and
                             ^
In file included from /home/wthrowe/spectre/src/Utilities/ConstantExpressions.hpp:12:0,
                 from /home/wthrowe/spectre/tests/Unit/Utilities/Test_ConstantExpressions.cpp:6:
/home/wthrowe/spectre/src/Utilities/Gsl.hpp:119:66: note: ‘constexpr const typename Cont::value_type& gsl::at(const Cont&, Size) [with Cont = std::array<long unsigned int, 3ul>; Size = long unsigned int; typename Cont::value_type = long unsigned int]’ is not usable as a constexpr function because:
 SPECTRE_ALWAYS_INLINE constexpr const typename Cont::value_type& at(
                                                                  ^

Experimenting, it doesn't like the use of strings and Parallel::abort in Expects. I believe these are supposed to be allowed as long as the Expects condition is true so they are not evaluated.

Unordered container stream output is unordered

Yes, this is expected, but it is perhaps not desirable. It makes writing tests of stream operators difficult (encountered in PR #100). It probably makes debugging harder.

We could fix this by changing our stream operators for the containers to print in some order. Since we can't assume the elements can be ordered, the easiest way to do this is probably to convert them all to strings and then sort those.

Tests sometimes time out

The SpECTRE tests have sometimes been timing out on Travis and (until recently) on my workstation as well. The changes in #89 worked around this issue by increasing the allowed duration of each test, but the underlying problem is still not understood.

I am making this issue so we keep awareness of this problem. A satisfactory resolution would involve understanding why tests sometimes time out and hopefully avoiding the workaround of #89.

Some further details given below:

---

On July 3rd, @nilsdeppe and I discussed the tests occasionally timing out on Travis and my workstation. Some experiments on my workstation suggested:

• the timeout affects, on average, roughly 1 test other every time all tests are run (at the time there were ~100 tests)
• there is no obvious pattern indicating which tests time out vs. which tests do not

Since then I have done a Linux update on my workstation and I no longer see any tests timing out, suggesting a possible link to low-level libraries or perhaps the kernel.

Documentation improvements

Going through https://sxs-collaboration.github.io/spectre/installation.html, and listing everything that's unclear

• GCC 5.2 or later, or Clang 3.6 or later: Add information for mac (Nils verbally: LLVM 8.1 should be ok)
• mention that hdf5 needs to be compiled without mpi (e.g. in brew do not use the --with-mpi option)
• For download Charm++, state that it needs to be compiled from source (for patching). Also mention that if compelling on a local laptop, easiest is to do cd charm; ./build for the interactive built, where default options are mostly ok. State that precisely v6.7.1 is needed for MacOS
• mention to clone master of brigand, Catch and yaml-cpp
• When compiling bitbucket/dg-charm:spectre_2b according to the instructions in the GitHub repo, several issues arise
  1. apparently, Fortran is only capitalized in -D CMAKE_Fortran_COMPILER
  2. my charm build-directory is charm/multicore-darwin-x86_64, which is not captured by the glob "path/to/charm/build-*"
  3. add info about patching charm++
  4. add info about how to specify the paths to the other packages that were installed earlier
  5. and/or add info about slack

Make Travis check that we do not use Catch's default Approx

Once PR #84 is closed, we want our tests to use our default tolerance instead of Catch's.

This issue is a reminder to have Travis check we do not use Approx in tests.

SegmentId and ElementId Should be noexcept

All of SegmentId and ElementId can be noexcept.

Reorder Travis tasks

For some reason Travis has decided to run its tasks in serial for my pull request, which means I don't get clang-tidy output until waiting through the OSX queue. Can we put the OSX tasks at the end?

make a good way to iterate over Directions

We would like a good way to iterate over each Direction

We should warn about unknown `[[Tags]]` on tests

Currently they are ignored and all the used ones are optional, so there's no protection against typos or missed updates from the old code.

Linux + clang build failure

I am trying to compile SpECTRE with clang on a Linux machine. The machine is set up according to the installation notes:

• llvm & SpECTRE deps installed using Spack
• Charm++ compiled using ./build charm++ multicore-linux64 clang -j8 --with-production and patched

I am running the following CMake command for SpECTRE:

cmake -D CMAKE_CXX_COMPILER=clang++ -D CMAKE_C_COMPILER=clang -D CMAKE_Fortran_COMPILER=gfortran -D CHARM_ROOT=/home/francois/code/charm/multicore-linux64-clang ..

I get the following error:

[snip]
-- CMAKE_BUILD_TYPE: Debug
CMake Error at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find LIBCXX (missing: LIBCXX_INCLUDE_DIRS)
Call Stack (most recent call first):
  /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:377 (_FPHSA_FAILURE_MESSAGE)
  cmake/FindLIBCXX.cmake:68 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  cmake/SetupLibCXX.cmake:5 (find_package)
  CMakeLists.txt:31 (include)

I am able to compile other projects with clang++ -stdlib=libc++ ..., so the library should be findable.

cmake fails if coverxygen is not found

Coverxygen is listed as an optional package and should not cause cmake to fail if it is not found

exclude some dirs from coverage

Exclude src/Executables from both code and doc coverage
Exclude tests from doc coverage

Requires cause Travis CHECK_FILES check to always fail

Every pull request fails checks with

Found lines over 80 characters:
./src/Utilities/Requires.hpp:14:  using template_error_types_failed_to_meet_requirements_on_template_parameters =

No way to prevent compiler flag overriding

It seems that our build system will always either append flags to enable assertions or append flags to enable optimizations, and these flags are placed at the end of the compiler invocation so they cannot be overridden. This makes trying to debug compiler-flag-related issues really annoying, since you can't control the compiler flags.

The internet suggests setting neither Debug nor Release as the build type to do this, but our system checks for that and dies if you do it.

cmake list does not append to lists in the parent scope

The cmake command

list(APPEND A_LIST STUFF_TO_APPEND)

will only append to A_LIST in the current scope.

We need to fix this in multiple locations for adding external libraries.
The proposed fix is to define a new cmake function

append_to_list(...)

which uses the cmake function set which can modify a variable in the PARENT_SCOPE

Missing Parentheses Warning in some CHECK statements

In some cases the CHECK statements supplied by Catch result in the compiler warning about missing parentheses. The cause if this is that the CHECK macro expands to (in v1.9.6 of Catch)

try { \
  CATCH_INTERNAL_SUPPRESS_PARENTHESES_WARNINGS \
  ( __catchResult <= expr ).endExpression(); \
  CATCH_INTERNAL_UNSUPPRESS_PARENTHESES_WARNINGS \
} \

with

#       define CATCH_INTERNAL_SUPPRESS_PARENTHESES_WARNINGS \
            _Pragma( "clang diagnostic push" ) \
            _Pragma( "clang diagnostic ignored \"-Wparentheses\"" )
#       define CATCH_INTERNAL_UNSUPPRESS_PARENTHESES_WARNINGS \
            _Pragma( "clang diagnostic pop" )

I have a few questions.

1. Which compiler versions are producing this error?

2. @wthrowe you said there is a missing pop. Where did you see this?

Note: We first encountered this with PR #82

Have Travis run Cppcheck

It would be very useful to have TravisCI run Cppcheck over the code for all PRs to decrease what we need to catch in code review. This should be relatively straightforward to do by adding to one of the existing build targets.

serialize_and_deserialize should do the test or have a wrapper that does

It's causing confusion, since it doesn't behave like the other helpers. Serialization is a copy operation, so by our guidelines any concrete classes that support it should have equality defined.

Git hooks can go wrong after some git operations.

This ticket is to document an issue I encountered, but am not sure how to reproduce:

Bad git operations can leave the git hooks in a state that prevents normal work in the repository

Here's what I know:

I (somehow) messed up an interactive rebase, and attempted to reset to a pre-rebase state with git reset --hard HEAD@{N}. The tracked files were restored as expected.

However, I was then unable to run most git commands (e.g. git commit --amend) due to two files triggering the pre-commit hook for having lines over 80 chars. These files are:

build/CMakeFiles/3.8.0/CompilerIdCxx/CmakeCXXCompilerId.cpp
tests/Unit/Numerical/Spectral/Test_DefiniteIntegral.cpp

It is true that both of these files have lines over 80 chars (*). But as I had no local changes to these files, I would not expect the git hooks to prevent me from committing other changes. Ultimately I resolved the problem by rm -r .git/hooks. I suspect a cached file in this directory ended up in a bad state during the broken rebase/reset process.

If anyone else encounters this problem, please comment with any further information!

---

(*) The CMake file is not tracked by the repo, but the test is. This brings up two secondary issues:

1. The git hook should probably not trigger on a CMake-generated file.
2. How did our test get past the 80-char git hooks??

Checking -Wno-* support doesn't work on gcc

We need to check support for the positive warning flag to determine whether the negative one works.

See http://gcc.gnu.org/wiki/FAQ#wnowarning

wrong include in InfoFromBuild.hpp

Should include but not

Remove NOLINT comments from move semantics

A simple git grep 'default; // NOLINT' should get almost all of these, also any on PUP functions because we need to be able to return things by references and updated our clang-tidy config

code guidelines additions

• unused function parameters should be /* parameter_name */
• explain when variables/types/templates end with _t
• change "exactly 70 =" to "exactly 64 ="
• rename "Types NOT specified in the name " -> "Do NOT encode type-information in variable name"
• rename "snake_case: (meta)function and (meta)variables names." -> snake_case: functions, variables, meta-functions and meta-variables" [rationale: "(meta)XXX" is less familiar and therefore less clear]

port options

Will is doing this in #142

max size of LegendreGaussLobatto::grid_points_to_spectral_matrix

LegendreGaussLobatto::grid_points_to_spectral_matrix(size_t n)

assumes a maximum size for n. If a larger number is passed in, a runtime error occurs triggered by std::map.at(). Make the error message comprehensible. Or, even better. Remove the hardcoded upper limit.

Have Travis search for `<iostream>` header

We do not want the <iostream> header in the repository because std::cout will not work well with Charm++, and Parallel::printf should be used instead.

Note: This is build system related

Setup Directory Structure

Add root level directories for cmake, docs, external, src, tests, and tools

Should test_move_semantics take by value?

It would avoid the issue that its argument is moved out of but this is not obvious (as caused issues in pull request #82), but it would mean that some of the static_asserts in it become not-useful since you already need to do a move to call the function.