Add CCE system equations #1397

moxcodes · 2019-02-26T19:24:53Z

Proposed changes

This PR adds the Cauchy characteristic extraction system equations, which compute the set of inputs to the hypersurface integration routine, and represent the core of the physics at work here.

The equations I've implemented are complicated and have been adjusted to the computational task from the equations in the literature. If any of the reviewers want to be thorough and check the form of the equations themselves (and not simply trust the forms in the documentation), please just leave a comment to that effect and I will create a repository which contains my notes and Mathematica document to produce these results. I would not recommend any reviewer investing the time needed to verify the form of the equations without those reference materials.

Types of changes:

New feature

Component:

Code
Documentation

Code review checklist

The PR passes all checks, including unit tests, clang-tidy and IWYU.
For instructions on how to perform the CI checks locally refer to the Dev
guide on the Travis CI.
The code is documented and the documentation renders correctly. Run
make doc to generate the documentation locally into BUILD_DIR/docs/html.
Then open index.html.
The code follows the stylistic and code quality guidelines listed in the
code review guide.

Dependencies

Add support for ComplexDataVector to Pypp tests #1398

nilsdeppe · 2019-02-26T19:31:38Z

tests/Unit/Pypp/PyppFundamentals.hpp

@@ -212,6 +212,32 @@ struct ToPyObject<DataVector, std::nullptr_t> {
  }
 };

+template <>


I'd recommend factoring this commit out since it'll be easier to get in and should have different reviewers

That makes good sense 👍. I was struggling with whether I should ask @osheamonn for this one because of this change.

hrueter

group review by @nilsleiffischer (main), @hrueter
We did not yet review the full PR.

If you think it would be hard to follow the calculation in the documentation please either make the documentation self-contained or add your notes as TeX files to the /docs. They are not yet automatically built when building the documentation. If you think it makes sense to add provide the Mathematica notebooks in the documentation, we propose to publish them in a separate repository. Assign them a DOI and link them from our documentation. (use e.g. zenodo )

docs/References.bib

hrueter · 2019-03-04T10:13:22Z

src/Evolution/Systems/Cce/Tags.hpp

+  static std::string name() noexcept { return "Beta"; }
+};
+
+/// Bondi parameter \f$J\f$


J -> H

What is H actually? We can't find it in the Handmer paper.

Ah, apparently notation has diverged for this quantity, and the Handmer paper doesn't discuss the procedure in detail. I'll add more explanatory detail to this comment 👍.

hrueter · 2019-03-04T10:14:17Z

src/Evolution/Systems/Cce/Tags.hpp

+};
+
+/// Bondi parameter \f$J\f$
+struct H : db::SimpleTag {


We suggest naming these BondiH, BondiJ, ...

I have a mild preference for keeping the tags brief, due to the effect of expanding the already very long tag lists in Equations.hpp. The names are consistently used in CCE, and anything that accesses these tags outside the CCE module will need to access them by Cce::Tags::H etc, so I think it is sufficiently disambiguated.

I’d still prefer BondiH, BondiJ etc, just to avoid single-letter type names that have the possibility of being hard to search-and-replace. I’d also like the other reviewers @markscheel @nilsdeppe to weigh in here before you make the change.

Yea, I hate single letter things. With tags I don't feel as strongly because they're always prefixed with Tags::, but I still am not a fan of it. Since currently there apparently is a comment "Bondi parameter tags` necessary, @nilsleiffischer's suggestion also removes the need for that comment.

hrueter · 2019-03-04T10:18:45Z

src/Evolution/Systems/Cce/Tags.hpp

+/// A prefix tag representing a quantity that will appear on the right-hand side
+/// of an explicitly regular differential equation
+template <typename Tag>
+struct IntegrandFor : db::PrefixTag, db::SimpleTag {


remove For. This is implied by the prefixing.

src/Evolution/Systems/Cce/Tags.hpp

hrueter · 2019-03-04T10:25:10Z

src/Evolution/Systems/Cce/Tags.hpp

+/// represent some of the cached quantities for CCE. Note that the spins are
+/// added.
+template <typename LhsTag, typename RhsTag>
+struct Times : db::PrefixTag, db::SimpleTag {


This looks like being general to spin weighted quantities. So better move to DataStructures/Tags.hpp or DataStructures/SpinWeighted.hpp?

Or perhaps wait until this is needed elsewhere?

hrueter · 2019-03-04T10:36:11Z

src/Evolution/Systems/Cce/Tags.hpp

+/// Temporary scalars for particular spin weights.
+template <size_t index, int spin>
+struct TemporarySpinWeightedScalar : db::SimpleTag {
+  using type = Scalar<SpinWeighted<ComplexDataVector, spin>>;


We propose to add a spinweighted prefix tag to DataStructures/Tags.hpp, that swaps the type of a tensor with a spinweighted type. Then you can use the TempBuffer with the aliases from Datastructures/Variables.hpp.

hrueter · 2019-03-04T10:37:33Z

src/Evolution/Systems/Cce/Tags.hpp

+};
+
+// Additional values which are frequently used in CCE calculations, and
+// therefore worth caching


Should these go into a detail namespace? Maybe in a separate file?

I don't think we've put tags into detail namespaces very often (ever?). My issue with moving these into a detail namespace is that they then won't show up in Doxygen and nobody will be able to find them without reading the code.

nilsvu · 2019-03-05T08:46:08Z

src/Evolution/Systems/Cce/Equations.hpp

+  template <typename BondiAndSWCacheTagList, typename DerivativeTagList,
+            typename IntegrandTagList, typename SecondaryTagList,
+            typename TemporaryTagList>
+  void evaluate(


As discussed at the call yesterday, a comment of ours for some reason didn't make it into the review. We suggested to make this conform to the existing db::mutate_apply mechanism, see also #1413 (that PR only makes that mechanism easier to use, it works already fine right now). Let's talk in a hangout if anything is unclear.

fmahebert

Review with @markscheel (primary) and @moxcodes. We looked at the Tags.hpp and did a brief overview of the equations. Still more review needed.

fmahebert · 2019-03-05T23:54:32Z

src/Evolution/Systems/Cce/Tags.hpp

@@ -0,0 +1,255 @@
+// Distributed under the MIT License.  See LICENSE.txt for details.


Should be two (2) lines

... how did this even get committed?

fmahebert · 2019-03-06T00:02:04Z

src/Evolution/Systems/Cce/Tags.hpp

+};
+
+/// Bondi parameter \f$K^2 = 1 + J \bar{J}\f$
+struct KSquared : db::SimpleTag {


@moxcodes notes to self: check if this is used.

fmahebert · 2019-03-06T00:09:07Z

src/Evolution/Systems/Cce/Tags.hpp

+struct Dy : db::PrefixTag, db::SimpleTag {
+  using type = Scalar<SpinWeighted<ComplexDataVector, Tag::type::type::spin>>;
+  using tag = Tag;
+  static const size_t dir = 2;


Please use a clearer name. Suggest: dimension_to_differentiate

Perhaps open an issue to add documentation that an assumption is made about the (phi, theta, r) ordering of the CCE coordinates.

Issue created and assigned to me.

fmahebert · 2019-03-06T00:24:55Z

src/Evolution/Systems/Cce/Tags.hpp

+
+/// A prefix tag representing the contribution to a differential equation which
+/// has a contribution of a nontrivial linear operator in addition to the
+/// differential operator


For ease of reading, suggest: "A prefix tag representing a linear operator that acts on Tag"

Also include the equation in the documentation (here? elsewhere?), because the tag is specialized to a particular differential equation (note, e.g., the hardcoded spin).

fmahebert · 2019-03-06T00:25:32Z

src/Evolution/Systems/Cce/Tags.hpp

+
+/// A prefix tag representing the contribution to a differential equation which
+/// has a contribution of a nontrivial linear operator acting on the conjugate
+/// of the quantity being solved for in addition to the differential operator


"A prefix tag representing a linear operator that acts on the conjugate of Tag"

fmahebert · 2019-03-06T00:41:03Z

src/Evolution/Systems/Cce/Tags.hpp

+};
+
+/// The value \f$\exp(2\beta)\f$, which is used frequently in the Bondi
+/// computations.


Drop second half of comment, it isn't very illuminating. Same on following tag.

fmahebert · 2019-03-06T00:43:09Z

src/Evolution/Systems/Cce/Tags.hpp

+
+/// The value \f$ J (\bar{Q} - 2 \bar{\eth} \beta ) \f$, which must be computed
+/// as an intermediate step to CCE equations.
+struct JbarQMinus2EthBeta : db::SimpleTag {


Convention so far has been to use tag name Xbar for \bar{X}, so fix doxygen and/or tag name for consistency. Or rename to SadTag.

fmahebert · 2019-03-06T00:48:28Z

src/Evolution/Systems/Cce/Equations.hpp

+/// \brief A struct for providing a tmpl::list of integrand tags that need to be
+/// computed before integration can proceed for a given Bondi variable tag.
+template <typename BondiVariable>
+using integrands_to_compute_for_bondi_variable =


integrands -> integrand_terms (globally)

fmahebert · 2019-03-06T00:55:04Z

src/Evolution/Systems/Cce/Equations.hpp

+};
+template <>
+struct tags_needed_for_integrand_computation_impl<
+    Tags::IntegrandFor<Tags::Beta>, TagsCategory::Secondary> {


Secondary -> IndependentOfIntegration or similar

fmahebert · 2019-03-06T00:59:08Z

src/Evolution/Systems/Cce/Equations.hpp

+
+template <>
+struct tags_needed_for_integrand_computation_impl<
+    Tags::IntegrandFor<Tags::Beta>, TagsCategory::BondiAndSWCache> {


BondiAndSWCache -> TagsToSwshDifferentiate

nilsvu

I think the move of the implementations into the cpp file worked out great 👍 A few more comments on the overall structure below. Also I think it would be good to "activate" the second reviewer, to get a second opinion and make sure we are converging to a state that will be merged. @markscheel would you review this?

nilsvu · 2019-03-08T09:43:36Z

docs/References.bib

@@ -169,6 +169,37 @@ @book{Hartle2003gravity
  url        = "https://www.pearson.com/us/higher-education/program/Hartle-Gravity-An-Introduction-to-Einstein-s-General-Relativity/PGM298455.html",
 }

+@article{Bishop1997ik,


Please sort alphabetically (see guidelines linked at the top of this file)

src/DataStructures/Tags.hpp

nilsvu · 2019-03-08T09:47:23Z

src/Evolution/Systems/Cce/Equations.cpp

+
+namespace Cce {
+// suppresses doxygen problems with these functions
+///\cond


add space /// \cond, also you don't need the explanatory comment, this is done at a lot of places in the code

This one I felt should have a comment, as dox is usually not rendered for cpp, but there appears to be a small bug in doxygen tackling this file.

nilsvu · 2019-03-08T10:05:30Z

src/Evolution/Systems/Cce/Equations.hpp

+  // clang-tidy readability-avoid-const-params-in-decls, but the consts must
+  // stay
+  static void apply(
+      const gsl::not_null<SpinWeighted<ComplexDataVector, 0>*>  // NOLINT


I think you can remove the consts here, you only need them in the cpp.

That is not true for this scenario, because the calling interface in this case needs to know that it is passing to a function with const arguments (otherwise it can't rely on the function it calls to not mutate its own const arguments). At any rate, I wanted to avoid the long list of nolint's and it does not compile without the const :P

Modulo a compiler bug this is not how const works. You can safely remove the const on the not_null arguments in the forward declarations, there's no need to NOLINT anything. On the arguments that are references, the const matters. I tested this that it compiles the Cce and Test_Cce libraries clang7 and gcc6. We also do not const the not_null arguments everywhere else in the entire repository and have never had to NOLINT anything. We actually use not_null so we don't have to NOLINT.

Fortunately, this also can be reverted to standard use now that the Variables interface has been dropped.

nilsvu · 2019-03-08T10:10:01Z

tests/Unit/Evolution/Systems/Cce/TestFunctions.py

@@ -0,0 +1,195 @@
+# Distributed under the MIT License.


As of the guidelines introduced in #1382 please name the Python file the same as the cpp test file, without the Test_ prefix.

src/Evolution/Systems/Cce/Equations.hpp

nilsvu · 2019-03-08T10:23:14Z

src/Evolution/Systems/Cce/Equations.hpp

+ * `apply` function in the specialization of `ComputeBondiIntegrand`.
+ */
+template <typename IntegrandTag>
+struct MutateComputeBondiIntegrand {


name ComputeBondiIntegrand

nilsvu · 2019-03-08T10:27:02Z

src/Evolution/Systems/Cce/Equations.hpp

+ * - `temporary_cache` : `Temporary`
+ */
+template <typename IntegrandTag>
+struct ComputeBondiIntegrandFromVariables {


I suggest to move this into test helpers. If we end up needing this in the public interface we can always move it back, but it is not clear to me that we do.

I'd still lean towards keeping at as part of the public interface, if only because the CCE/SWSH interfaces are currently working with the design goal of having interfaces which act on clusters of tags with some mathematical and functional commonality, in order to manage the combinatorically large number of values that appear in the calculations and impose some mild hierarchy. It ends up having a nice connection as well to the way the lists of tags are presented in this namespace, where if the tags were all put into a single large list, it would be more difficult to read and maintain.

If there is consensus that the Variables interface should not be included in src, I will move it to the tests.

Another alternative is to make the templates more general still, and permit them to take an arbitrary template, so this code is equally operable for sets of Variables or TaggedTuples.

I'd like to get input from the other reviews on this. My intention here is to not make the interface unnecessarily complex before we know we need that functionality.

Alright, I'm happy to have other input for this one.

My main point is just that while it's conceptually simple to just take a bunch of arguments, it will tend to be a frustrating interface to mechanically use for calling code for the large number of arguments for some of these.

@hrueter @fmahebert @markscheel opinions?

Right, so I think we agree that calling these functions with a list of arguments is conceptually nice and clean, but cumbersome when one has to write it out. That is, however, why we have the struct with the argument_tags typelist, so that you can just hand the struct over to the DataBox and have it sort out the arguments for you. I suggest you look at the GeneralizedHarmonic code and tests for an example. Those are just structs with a plain argument_tags list and an apply function that takes those arguments.

When we discussed this last week you mentioned you are worried about having to call the functions explicitly in tests, which is why I suggested to move this "short-cut" code with the categories and Variables to the tests.

From separate discussion, we concluded that it will be best to move these to the test helpers for this PR as suggested by @nilsleiffischer .

nilsvu · 2019-03-08T10:29:43Z

Oh @markscheel sorry I didn't realised you already reviewed this as part of the group review posted by @fmahebert

moxcodes · 2019-03-08T17:55:18Z

Fixups posted for all points aside from moving the Variables to a test helper. The last point, I'll implement and post separately the chosen solution once the discussion is resolved.
Thanks for the reviews!

kidder · 2019-03-11T22:51:21Z

Current status:

@hrueter and @nilsleiffischer to look at latest fix ups
@markscheel and @fmahebert to look at latest fix ups

nilsvu

Please squash the fixups and these minor additional changes

nilsvu · 2019-03-12T17:45:48Z

docs/References.bib

@@ -200,6 +190,16 @@ @article{Handmer2014qha
  url            = "https://doi.org/10.1088/0264-9381/32/2/025008"
 }

+  @book{Hartle2003gravity,


remove the leading whitespace please

nilsvu · 2019-03-12T17:48:39Z

src/Evolution/Systems/Cce/Equations.hpp

- * \brief An interface to ComputeBondiIntegrand which is convenient for passing
- * around clusters of bondi quantities in `Variables`
+ * \brief An interface to ComputeBondiIntegrandImpl which is convenient for
+ * passing around clusters of bondi quantities in `Variables`


nilsvu · 2019-03-12T17:49:58Z

src/Evolution/Systems/Cce/Equations.hpp

+#ifndef __clang__
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wunused-but-set-parameter"
+#endif  // __clang__


I don't know if this is the best-practice way to do this, so perhaps another reviewer can confirm

Do you mean specifically the #ifndef __clang__ to select compiler? This was the method I was advised to use for a compiler-specific warning in #1145 , though the warning was ultimately removed by other changes, but I'm happy to entertain other methods.

moxcodes · 2019-03-13T20:40:29Z

src/Evolution/Systems/Cce/Equations.hpp

+          IntegrandTag, TagsCategory::IndependentOfBondiIntegration>>;
+
+  template <typename... Args>
+  static void apply(Args... args) noexcept {


const & or &&

moxcodes

A summary of the conclusions from discussion with @nilsdeppe and @nilsleiffischer for the last few changes to these utilities.

moxcodes · 2019-03-13T21:07:09Z

src/Evolution/Systems/Cce/Equations.hpp

+ * - `temporary_cache` : `Temporary`
+ */
+template <typename IntegrandTag>
+struct ComputeBondiIntegrandFromVariables {


From separate discussion, we concluded that it will be best to move these to the test helpers for this PR as suggested by @nilsleiffischer .

moxcodes · 2019-03-13T21:08:22Z

src/Evolution/Systems/Cce/Equations.hpp

+ * `SpinWeighted<ComplexDataVector, N>`s for each of the Bondi arguments.
+ */
+template <typename IntegrandTag>
+struct ComputeBondiIntegrandImpl;


The convenience impl should instead be a impl function moved in the ComputeBondiIntegrand function.

moxcodes · 2019-03-13T21:09:42Z

src/Evolution/Systems/Cce/Equations.hpp

+
+// - Tags for Linear operator that acts on the conjugate of H
+template <>
+struct tags_needed_for_integrand_computation_impl<


These type aliases, for each term, should be a single struct, and the collections of tags should probably be placed in a separate file.

moxcodes · 2019-03-13T21:13:28Z

tests/Unit/Evolution/Systems/Cce/Test_Equations.cpp

+          using bondi_integrand_tag = typename decltype(y)::type;
+          using bondi_integrand_argument_list =
+              all_arguments_for_integrand<bondi_integrand_tag>;
+          // check the Variables interface


Once the Variables interface is in a test helpers, this comment should give a few more details as to why we are testing this way (i.e. to allow the test to use the equations without using a databox and without manually specifying each of the functions and each of the numerous arguments for each of those functions).

moxcodes · 2019-03-20T20:59:44Z

Fixups posted and rebased. The only remaining fixup commit is the one regarding the discussion about the Variables helper and the accompanying typedefs.

nilsvu · 2019-03-21T05:37:06Z

src/Evolution/Systems/Cce/Equations.hpp

+class ComplexDataVector;
+///\endcond
+
+/// Contains system-specific tools for Cauchy Characteristic Extraction.


What do you mean with system-specific here? Perhaps just “functionality” for CCE

nilsvu · 2019-03-21T05:39:31Z

src/Evolution/Systems/Cce/Equations.hpp

+ * Characteristic hypersurface equations.
+ *
+ * \details The template argument must be one of the integrand-related prefix
+ * tags applied to a Bondi quantity for which that integrand is required. The


The template argument is not the prefix tag itself, but a prefixed tag, if I understand correctly

I'll adjust the later part of the sentence to refer to 'prefix tags templated on a Bondi quantity tag' for improved clarity.

nilsvu · 2019-03-21T06:17:32Z

src/Evolution/Systems/Cce/Equations.hpp

+          Tags::Integrand<Tags::Beta>>,
+      swsh_derivative_tags_needed_for_integrand<Tags::Integrand<Tags::Beta>>,
+      integration_independent_tags_needed_for_integrand<
+          Tags::Integrand<Tags::Beta>>>;


To build these return and argument tags you now refer to the tags lists in the other file. From our discussion the other day I understood we’d go with another strategy, and if I misunderstood then I’d like to suggest it here:

Build the tag lists self-contained within the specialization of ComputeBondiIntegrand. You can split it into buffer_tags or similar, possibly private, type lists to retain the categoristation, if you like.

Remove the TagsLists.hpp from this PR, but keep the code around on another branch in case you’ll ever need it.

Also remove the integrand_terms_to_compute_for_bondi_variable from this PR but keep the code on another branch. It is part of the code that does the computation, in my opinion, so add it in a PR that makes use of it.

Before you make this change, I’d like the other reviewers to confirm @markscheel @nilsdeppe, so that you don’t waste time

Provided consensus, I'm fine doing the first two. I'd prefer, however, to keep the integrand_terms_to_compute_for_bondi_variable. It's actually used in the test, and without it being present here, I'd just have to reproduce the same set of types in the test either by direct listing inline or via type aliases there.

src/Evolution/Systems/Cce/Tags.hpp

nilsvu · 2019-03-21T06:25:39Z

src/Evolution/Systems/Cce/Tags.hpp

+  static std::string name() noexcept { return "Beta"; }
+};
+
+/// Bondi parameter \f$H = \partial_u J\f$. Note: the notation in the literature


\brief and \note

nilsvu · 2019-03-21T06:38:41Z

tests/Unit/Evolution/Systems/Cce/Equations.py

+    jbar = np.conj(j)
+    k_squared = (1.0 + j * jbar)
+    qbar = np.conj(q)
+    script_av = eth_beta * eth_jbar + ethbar_ethbar_j / 2.0 \


You take this as an argument but then compute it

It is a temporary buffer, taken by argument to avoid allocation.

but this is in python, so does that actually work?

oh, oops, I failed to note which file this was commenting on. No, it doesn't have any optimization utility in python. I could name the intermediate step something else, but I still need the argument so that the function signature is the same as the C++, and I still want to compute the intermediate step so that the function is reasonably readable.

Maybe there is a way to comment out the name of the argument like there is in C/C++? Or maybe just name that argument something that makes it clear it's not used?

You can use an underscore _ for unused arguments, that's the convention

nilsvu · 2019-03-21T06:43:31Z

tests/Unit/Evolution/Systems/Cce/Test_Equations.cpp

+int assign_to_tag(
+    const gsl::not_null<Variables<VariablesTagList>*> all_variables,
+    const Arg arg) noexcept {
+  get(get<Tag>(*all_variables)).data() = arg;


Why do you need the data()? Looks dangerous :D

This is a result of the SpinWeighted wrapper. The outer get obtains a SpinWeighted<ComplexDataVector, N>, and the .data() obtains the ComplexDataVector, which can be assigned regardless of the spin-weight. It's not a pointer, which I suspect is why it feels concerning.

nilsvu · 2019-03-21T06:44:08Z

tests/Unit/Evolution/Systems/Cce/Test_Equations.cpp

+template <typename Tag, typename VariablesTagList, typename Arg>
+int assign_to_tag(
+    const gsl::not_null<Variables<VariablesTagList>*> all_variables,
+    const Arg arg) noexcept {


nilsvu · 2019-03-21T06:46:22Z

tests/Unit/Evolution/Systems/Cce/Test_Equations.cpp

+    // create a variables which will hold everything
+    Variables<
+        tmpl::flatten<tmpl::append<ArgumentTagList, tmpl::list<OutputTag>>>>
+        all_variables{NumberOfGridPoints, std::complex<double>(10, 57)};


You can remove the second argument to construct with NaNs

nilsvu · 2019-03-21T06:52:13Z

src/Evolution/Systems/Cce/Equations.hpp

+          integrand_for_beta,
+      const SpinWeighted<ComplexDataVector, 2>& dy_j,        // NOLINT
+      const SpinWeighted<ComplexDataVector, 2>& j,           // NOLINT
+      const SpinWeighted<ComplexDataVector, 0>& one_minus_y  // NOLINT


You have to make sure that the apply function actually take the types of the return and argument tags. Since you want to avoid the many gets to bypass the Scalars, you’ll need an apply and apply_impl or similar, I think @nilsdeppe suggested something.

Ah, true. I agree this needs to be fixed.

To make sure this works, I think it would be good to add a test that calls db::mutate_apply. This will be made easier with #1413

moxcodes · 2019-04-01T21:58:35Z

new fixups posted. I believe all comments have been addressed aside from expanding the single-character bondi tags to BondiH etc. pending input from @markscheel or @fmahebert . I want to request that @nilsdeppe at least reads the new commit for the compile-time Tensor get for not_null pointers, as it alters Tensor.hpp.

moxcodes · 2019-04-02T05:52:53Z

squashed in a fix to get gcc to build

nilsvu · 2019-04-26T15:26:17Z

@moxcodes I'll be on vacation 1st to 11th of May, so unable to review this in that time. As far as I understand this is still waiting on input by @markscheel, @fmahebert and @nilsdeppe though. If you end up moving this PR forward while I'm away, please feel free to replace me with another reviewer.

nilsdeppe · 2019-05-14T16:13:03Z

@moxcodes could you please rebase and squash this, then ping me so I know to look at it? Thanks :)

moxcodes · 2019-05-14T16:52:29Z

@nilsdeppe I've squashed the previous fixes and rebased. I've added one fixup in which I remove the Variables business. After the refactor of other utilities, I've seen the light and managed to use the rest of the cce tools in ways more consistent with the DataBox models.

nilsdeppe · 2019-05-24T14:33:23Z

@nilsleiffischer @hrueter please give feedback on whether you're happy with the changes or not.

nilsvu

I'm quite happy about how this code is structured now :) thanks for your patience!

Only a few minor things:

src/Evolution/Systems/Cce/Tags.hpp

tests/Unit/Evolution/Systems/Cce/Equations.py

tests/Unit/Evolution/Systems/Cce/Test_Equations.cpp

moxcodes · 2019-05-30T00:31:54Z

@nilsleiffischer @fmahebert I've posted fixups for the last few comments.
Cheers

fmahebert · 2019-05-30T06:09:43Z

The fixups LGTM

nilsvu

This looks pretty much done to me. Sorry for not checking the fixups earlier, please rebase on develop and squash. Then, make sure Travis is happy and ping @sxs-collaboration/spectre-core-devs to get this finally merged :)

markscheel

All minor documentation changes.

I did not try to check the equations themselves (ugh); that's what the tests are for...

src/DataStructures/Tags.hpp

src/Evolution/Systems/Cce/Tags.hpp

src/Evolution/Systems/Cce/Equations.hpp

moxcodes · 2019-07-12T00:47:01Z

@markscheel I've posted fixups for all of the suggestions.
Cheers!

moxcodes · 2019-07-25T15:10:31Z

@markscheel Just waiting for the go-ahead before I squash the fixups.
Thanks!

markscheel · 2019-07-25T22:36:28Z

please squash!

- previously the Tags::SpinWeighted prefix tag and Tensors of spin-weighted quantities could not be easily put into a DataBox

- add tags for relevant CCE quantities - add computation methods for CCE integrands

moxcodes · 2019-07-25T23:15:53Z

squashed (and rebased)
Cheers!

kidder · 2019-07-26T02:10:08Z

@hrueter @nilsleiffischer @fmahebert @markscheel this should be ready for final approval. Please only request changes for obvious errors, anything else, just comment and @moxcodes can decide to address them or make an issue.

nilsvu

@moxcodes What I think is missing for these equations are tests that do not rely on the Python implementation, but work with some honest, hard-coded numbers that were computed externally. This is to make sure the C++ and Python implementation don't have the same bug, which can easily happen when coding them up. This was discussed briefly in this review before. Please consider adding such tests soon, perhaps make an issue for the record.

nilsvu · 2019-07-26T08:44:33Z

src/DataStructures/Variables.hpp

@@ -909,7 +909,7 @@ struct Subitems<TagList, Tag,

 namespace Tags {
 template <size_t N, typename T>
-struct TempTensor {
+struct TempTensor : db::SimpleTag {


@nilsdeppe is this resolved for you?

moxcodes · 2019-07-26T15:20:32Z

@nilsleiffischer This pull request alone doesn't have enough of the CCE infrastructure to have useful comparisons against hand-computed quantities. I agree that those checks are important (some already exist on my own branch), so I've created issue #1641 as a reminder to prioritize those tests once the CCE code is capable of them.

moxcodes · 2019-07-30T21:41:53Z

@sxs-collaboration/spectre-core-devs since @nilsleiffischer was the primary on the 'request changes' from @hrueter , I think this one is actually good to go?

@hreuter

@hreuter's comments were addressed and made as part of review led by @nilsleiffischer that @NilsL

moxcodes requested a review from markscheel February 26, 2019 19:24

moxcodes force-pushed the cce_equations branch from 7243b2a to 9dfb13f Compare February 26, 2019 19:30

nilsdeppe reviewed Feb 26, 2019

View reviewed changes

moxcodes force-pushed the cce_equations branch from 9dfb13f to 26f6ae2 Compare February 26, 2019 19:50

moxcodes added dependent Needs a different PR to be merged in first and removed dependent Needs a different PR to be merged in first labels Feb 26, 2019

hrueter previously requested changes Mar 4, 2019

View reviewed changes

nilsvu reviewed Mar 5, 2019

View reviewed changes

fmahebert suggested changes Mar 6, 2019

View reviewed changes

moxcodes force-pushed the cce_equations branch from 26f6ae2 to de74bad Compare March 8, 2019 07:43

nilsvu requested changes Mar 8, 2019

View reviewed changes

nilsvu requested changes Mar 12, 2019

View reviewed changes

moxcodes commented Mar 13, 2019

View reviewed changes

moxcodes force-pushed the cce_equations branch 2 times, most recently from d6e8f70 to 6e78272 Compare March 20, 2019 20:58

nilsvu requested changes Mar 21, 2019

View reviewed changes

moxcodes force-pushed the cce_equations branch from 22e234c to 4299029 Compare April 2, 2019 05:45

moxcodes force-pushed the cce_equations branch from 4299029 to 28f2a0f Compare April 27, 2019 23:09

moxcodes force-pushed the cce_equations branch 2 times, most recently from fcf0160 to 7d2e650 Compare May 14, 2019 16:50

moxcodes added a commit to moxcodes/spectre that referenced this pull request May 23, 2019

Add Cce Tags (shared with PR sxs-collaboration#1397)

3d8721f

nilsvu requested changes May 24, 2019

View reviewed changes

moxcodes dismissed nilsdeppe’s stale review via c63e2b9 May 30, 2019 00:31

nilsvu reviewed Jul 5, 2019

View reviewed changes

markscheel requested changes Jul 8, 2019

View reviewed changes

moxcodes force-pushed the cce_equations branch from e5a59aa to c1c5a27 Compare July 12, 2019 00:46

moxcodes force-pushed the cce_equations branch from c1c5a27 to 919e9e9 Compare July 12, 2019 01:30

moxcodes force-pushed the cce_equations branch from 919e9e9 to d52f3f8 Compare July 25, 2019 15:06

moxcodes force-pushed the cce_equations branch from d52f3f8 to 16313f0 Compare July 25, 2019 23:13

moxcodes added 3 commits July 25, 2019 18:14

Add CCE Tags

6f09892

Adjust Spin-Weighted Tag and struct for DataBox

5b56d65

- previously the Tags::SpinWeighted prefix tag and Tensors of spin-weighted quantities could not be easily put into a DataBox

Add CCE equations

c96946a

- add tags for relevant CCE quantities - add computation methods for CCE integrands

moxcodes force-pushed the cce_equations branch from 16313f0 to c96946a Compare July 25, 2019 23:14

fmahebert approved these changes Jul 26, 2019

View reviewed changes

nilsvu approved these changes Jul 26, 2019

View reviewed changes

markscheel approved these changes Jul 26, 2019

View reviewed changes

kidder merged commit a36f01e into sxs-collaboration:develop Jul 31, 2019

moxcodes added a commit to moxcodes/spectre that referenced this pull request Aug 23, 2019

Add Cce Tags (shared with PR sxs-collaboration#1397)

86375e3

moxcodes added a commit to moxcodes/spectre that referenced this pull request Sep 24, 2019

Add Cce Tags (shared with PR sxs-collaboration#1397)

5e0bbcf

		@@ -0,0 +1,255 @@
		// Distributed under the MIT License. See LICENSE.txt for details.

Add CCE system equations #1397

Add CCE system equations #1397

Conversation

moxcodes commented Feb 26, 2019 • edited

Proposed changes

Types of changes:

Component:

Code review checklist

Dependencies

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrueter left a 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

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

nilsvu Mar 5, 2019 • edited

Choose a reason for hiding this comment

fmahebert left a 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

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

nilsvu left a 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

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

Choose a reason for hiding this comment

nilsvu commented Mar 8, 2019

moxcodes commented Mar 8, 2019

kidder commented Mar 11, 2019 • edited by nilsvu

nilsvu left a 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

moxcodes left a 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

moxcodes commented Mar 20, 2019

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

moxcodes commented Feb 26, 2019 •

edited

nilsvu Mar 5, 2019 •

edited

kidder commented Mar 11, 2019 •

edited by nilsvu