sxs-collaboration / spectre Goto Github PK
View Code? Open in Web Editor NEWSpECTRE is a code for multi-scale, multi-physics problems in astrophysics and gravitational physics.
Home Page: https://spectre-code.org
License: Other
SpECTRE is a code for multi-scale, multi-physics problems in astrophysics and gravitational physics.
Home Page: https://spectre-code.org
License: Other
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.
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.
Currently the atan2
function does not work for DataVector
s. 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.
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);
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;
}
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.
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.
I was supposed to do this during PR #103, but I forgot.
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.
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.
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.
This can't be done until we port the make_items_pupable function is ported over
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.
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
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.
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.
The test labels for Definite Integral should be "[Numerical][Spectral][Unit]"
like the other spectral tests. @gsb76 this is your task to correct.
Should make variable names in macros be such that they will not shadow other variable names, eg avoid_name_collisions_PARSE_ERROR
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.
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.
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!
/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.
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.
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:
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.
Going through https://sxs-collaboration.github.io/spectre/installation.html, and listing everything that's unclear
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.
All of SegmentId
and ElementId
can be noexcept
.
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?
We would like a good way to iterate over each Direction
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.
I am trying to compile SpECTRE with clang on a Linux machine. The machine is set up according to the installation notes:
./build charm++ multicore-linux64 clang -j8 --with-production
and patchedI 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.
Coverxygen is listed as an optional package and should not cause cmake to fail if it is not found
Exclude src/Executables from both code and doc coverage
Exclude tests from doc coverage
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 =
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.
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
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.
Which compiler versions are producing this error?
@wthrowe you said there is a missing pop
. Where did you see this?
Note: We first encountered this with PR #82
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.
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.
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:
We need to check support for the positive warning flag to determine whether the negative one works.
Should include but not
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
Will is doing this in #142
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.
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
Add root level directories for cmake
, docs
, external
, src
, tests
, and tools
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.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.