Add tags and fluxes to the ForceFree evolution system #4344

yoonso0-0 · 2022-11-02T19:50:21Z

Proposed changes

Upgrade instructions

Code review checklist

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.
The PR lists upgrade instructions and is labeled bugfix or
new feature if appropriate.

Further comments

teukolsky · 2022-11-02T23:05:48Z

src/Evolution/Systems/ForceFree/Fluxes.hpp

+ * \f$e^{abcd}\f$ and \f$e^{ijk}\f$ are usual antisymmetric _symbols_ (which
+ * only have the value \f$\pm 1\f$) with 4 and 3 indices, respectively, with the
+ * convention \f$e^{0123} = e^{123} = +1\f$.
+ *


We use the (sensible) convention e_{0123}=+1!

Agree with @teukolsky!

@yoonso0-0, just so that I follow the convention here: e is just [abcd], and epsilon is the tensor, right?
So that epsilon_{0123} = -epsilon^{0123} = +1; but e_{0123} = e^{0123} = +1 ?

Ah! I missed the point that e was the permutation symbol and epsilon the tensor. It's best not to treat the indices of e as things that can be raised or lowered, so as to avoid confusion. A good notation is e=[\alpha \beta \gamma \delta], with [0123]=+1. Then there's no confusion: \epsilon_{\alpha\beta\gamma\beta]=1/\sqrt{\det{-g}} [\alpha\beta\gamma\delta].

@ermost correct :) @teukolsky sorry for the confusion! I'll update the docs.

ermost · 2022-11-03T01:56:45Z

src/Evolution/Systems/ForceFree/Fluxes.hpp

+ * \f$e^{abcd}\f$ and \f$e^{ijk}\f$ are usual antisymmetric _symbols_ (which
+ * only have the value \f$\pm 1\f$) with 4 and 3 indices, respectively, with the
+ * convention \f$e^{0123} = e^{123} = +1\f$.
+ *


Agree with @teukolsky!

@yoonso0-0, just so that I follow the convention here: e is just [abcd], and epsilon is the tensor, right?
So that epsilon_{0123} = -epsilon^{0123} = +1; but e_{0123} = e^{0123} = +1 ?

ermost · 2022-11-03T13:09:26Z

src/Evolution/Systems/ForceFree/Tags.hpp

+      "Options related to constraint damping"};
+  using group = ForceFreeGroup;
+};
+


The force-free current includes a damping term, that acts as an effective resistivity.
It might make sense to add an option for that, too. Sometimes this needs to be adjusted.
Eta might be a good name; it has the same dimensions as the Kappa's

Thanks for pointing this out!

From your paper I see Nu was used and in Alic's paper SigmaB was used. I think we can use Eta when we describe our equations, but would prefer some descriptive name inside the code.

something like RelaxationParameter? I cannot think of a good name on this, but I can put this parameter in a separate option group named like ForceFreeCurrent:

How about ParallelConductivity? Physically that parameter is the conductivity parallel to magnetic field lines.

Sounds nice 👍

yoonso0-0 · 2022-11-03T20:57:33Z

Rebased and pushed fixups

ermost

fixUps look good to me

wthrowe · 2022-11-08T19:55:08Z

src/Evolution/Systems/ForceFree/Fluxes.cpp

+namespace ForceFree {
+
+namespace detail {
+void fluxes_impl(


Change the detail namespace to an anonymous namespace. Alternatively, combine this function with the one that calls it.

@nilsdeppe I think I asked about this to you before, on separating _impl function inside a detail namespace. If I remember correctly this is for reducing memory allocations when computing the DG time derivative.

@wthrowe GRMHD is also using this _impl splitting

Yes, in the GRMHD system it's done because we need to compute the fluxes and sources on different grids with FD

wthrowe · 2022-11-08T20:02:55Z

src/Evolution/Systems/ForceFree/Fluxes.cpp

+
+    tilde_e_flux->get(j, i) += -get(lapse) * it.sign() *
+                               tilde_b_one_form.get(k) /
+                               get(sqrt_det_spatial_metric);


Move it.sign() to immediately after the -. This reduces the number of DataVector math operations by one. General rule is to put all non-DataVector factors first.

Optional: the one_form variables are only used in the combination lapse * one_form / sqrt_det_spatial_metric, so those factors could be included in the precomputed value.

Pushed a fixup for this.

yoonso0-0 · 2022-11-15T17:46:36Z

Rebased and pushed fixup. I interchanged Psi and Phi part in the docs to match the code arguments order

yoonso0-0 · 2022-11-16T03:13:20Z

@wthrowe I pushed another fixup taking care of what's addressed in #4381

wthrowe · 2022-11-17T20:08:59Z

Will this system use subcell? I'm not familiar enough with it to know what typical results look like.

nilsdeppe · 2022-11-17T20:20:04Z

Yes

wthrowe · 2022-11-17T20:37:27Z

OK, sounds good then. Go ahead and squash.

yoonso0-0 · 2022-11-20T01:25:30Z

Thanks for the review!

yoonso0-0 force-pushed the ffe/add_tags_and_fluxes branch 2 times, most recently from 95111cd to bcdfc3a Compare November 2, 2022 21:35

teukolsky reviewed Nov 2, 2022

View reviewed changes

ermost reviewed Nov 3, 2022

View reviewed changes

yoonso0-0 mentioned this pull request Nov 3, 2022

Add largest characteristic speed to FFE system #4346

Merged

4 tasks

yoonso0-0 force-pushed the ffe/add_tags_and_fluxes branch 2 times, most recently from f856de6 to 07fe16c Compare November 3, 2022 20:52

yoonso0-0 requested a review from ermost November 3, 2022 20:57

ermost previously approved these changes Nov 4, 2022

View reviewed changes

yoonso0-0 dismissed ermost’s stale review via b33772a November 4, 2022 16:27

yoonso0-0 force-pushed the ffe/add_tags_and_fluxes branch from 07fe16c to b33772a Compare November 4, 2022 16:27

yoonso0-0 mentioned this pull request Nov 4, 2022

Compute source terms in the ForceFree evolution system #4351

Merged

4 tasks

ermost previously approved these changes Nov 4, 2022

View reviewed changes

wthrowe requested changes Nov 8, 2022

View reviewed changes

yoonso0-0 dismissed ermost’s stale review via 0c5d991 November 8, 2022 22:54

yoonso0-0 force-pushed the ffe/add_tags_and_fluxes branch 2 times, most recently from 0c5d991 to e51c178 Compare November 8, 2022 23:51

yoonso0-0 mentioned this pull request Nov 11, 2022

Add Ohm's law for the GRFFE system #4370

Closed

4 tasks

yoonso0-0 force-pushed the ffe/add_tags_and_fluxes branch from e51c178 to 2c71146 Compare November 15, 2022 17:43

yoonso0-0 force-pushed the ffe/add_tags_and_fluxes branch from 2c71146 to df2aa9f Compare November 16, 2022 03:12

yoonso0-0 added 2 commits November 17, 2022 12:39

Add tags for the ForceFree evolution system

43213bd

Add fluxes to the ForceFree system

795c5dd

yoonso0-0 force-pushed the ffe/add_tags_and_fluxes branch from df2aa9f to 795c5dd Compare November 17, 2022 20:39

wthrowe approved these changes Nov 17, 2022

View reviewed changes

nilsdeppe approved these changes Nov 19, 2022

View reviewed changes

nilsdeppe merged commit b2ebf67 into sxs-collaboration:develop Nov 19, 2022

yoonso0-0 deleted the ffe/add_tags_and_fluxes branch November 20, 2022 01:25

Add tags and fluxes to the ForceFree evolution system #4344

Add tags and fluxes to the ForceFree evolution system #4344

Uh oh!

Conversation

yoonso0-0 commented Nov 2, 2022

Proposed changes

Upgrade instructions

Code review checklist

Further comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoonso0-0 commented Nov 3, 2022

Uh oh!

ermost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoonso0-0 commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yoonso0-0 commented Nov 16, 2022

Uh oh!

wthrowe commented Nov 17, 2022

Uh oh!

nilsdeppe commented Nov 17, 2022

Uh oh!

wthrowe commented Nov 17, 2022

Uh oh!

yoonso0-0 commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yoonso0-0 commented Nov 15, 2022 •

edited

Loading

yoonso0-0 commented Nov 20, 2022 •

edited

Loading