Correct tildeB sources and fluxes in ValenciaDivClean #5960
Conversation
There was a problem hiding this comment.
This still needs updates to the documentation (in src/Evolution/Systems/GrMhd/ValenciaDivClean/System.hpp as well as the Fluxes.hpp and Sources.hpp files) and the tests. I haven't checked these specific ones, but most of the tests for this sort of thing have the equations reimplemented in python, which should be the main thing that needs fixing there.
| @@ -67,7 +67,7 @@ void fluxes_impl( | |||
| tilde_b_flux->get(i, j) = | |||
| tilde_b.get(j) * transport_velocity->get(i) + | |||
| get(lapse) * (get(tilde_phi) * inv_spatial_metric.get(i, j) - | |||
| spatial_velocity.get(j) * tilde_b.get(i)); | |||
| transport_velocity->get(j) * tilde_b.get(i)); | |||
There was a problem hiding this comment.
Should no longer be multiplied by the lapse, I believe.
| get(lapse) * (get(tilde_phi) * inv_spatial_metric.get(i, j) - | ||
| spatial_velocity.get(j) * tilde_b.get(i)); | ||
| get(lapse) * (get(tilde_phi) * inv_spatial_metric.get(i, j)) - | ||
| transport_velocity->get(j) * tilde_b.get(i); |
There was a problem hiding this comment.
Reorder these terms to match the docs, just for readability.
| @@ -166,6 +167,9 @@ namespace ValenciaDivClean { | |||
| * divergence-free (no-monopole) condition \f$\Phi = \partial_i {\tilde B}^i = | |||
| * 0\f$. | |||
| * | |||
| * \note The fluxes and sources of {\tilde B} follow eq. 20 of | |||
| @@ -166,6 +167,9 @@ namespace ValenciaDivClean { | |||
| * divergence-free (no-monopole) condition \f$\Phi = \partial_i {\tilde B}^i = | |||
| * 0\f$. | |||
| * | |||
| * \note The fluxes and sources of {\tilde B} follow eq. 20 of | |||
| * \cite Palenzuela2018 rather than eq. 89 of \cite Moesta2014 | |||
There was a problem hiding this comment.
The equation numbers for both papers look wrong.
|
Can you change your wording in commits to be present tense? E.g. |
norafl
changed the title
There was a problem hiding this comment.
This looks good. Two more things: code formatting, which I've commented on as a line-comment, and squashing. This PR is pretty much an atomic change, so it should be squashed into a single commit. Google "git interactive rebase" if you don't know how to do that. As noted by @nilsdeppe, make sure your resulting commit message follows our guidelines.
I think the CI error is just some random failure, so ignore that if it doesn't happen again after you push the squashed version.
| spatial_velocity.get(j) * tilde_b.get(i)); | ||
| tilde_b.get(j) * transport_velocity->get(i) | ||
| - transport_velocity->get(j) * tilde_b.get(i) + | ||
| get(lapse) * (get(tilde_phi) * inv_spatial_metric.get(i, j)); |
There was a problem hiding this comment.
Run this though clang-format. Their documentation has instructions on how to use it through emacs or git: https://clang.llvm.org/docs/ClangFormat.html . (Our repo has style options configured in a top-level .clang-format file, so you shouldn't need to configure anything yourself.)
|
Formatting looks good now, but something went wrong during your rebase. Probably easiest to just sort it out in person tomorrow. |
Proposed changes
Corrects the sources and fluxes in magnetic field evolution.
Currently, the code has made the simplification
$$\partial_i({\tilde B}^j\beta^i) = {\tilde B}^j\partial_i\beta^i,$$ ${\tilde B}^j\partial_i\beta^i$ from the magnetic field source terms and adds a corresponding ${\tilde B}^j\beta^i$ to the corresponding flux terms. This handling is consistent with eq. 20 of https://arxiv.org/pdf/2004.00870 and eq. 32 of https://arxiv.org/pdf/1806.04182.
which can introduce errors in handling the divergence of the magnetic field. This update removes the resulting
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments