Subcell grmhd ghost data #3287
Subcell grmhd ghost data #3287
Conversation
fmahebert
left a comment
There was a problem hiding this comment.
Overall LGTM other than confusion about what variable u_i / v_i / v^i you actually mean in the docs 😛
| * \f$u_i\f$, magnetic field \f$B^i\f$, and the divergence cleaning field | ||
| * \f$\Phi\f$ on the subcells so they can be used for reconstruction. | ||
| * | ||
| * The computation copies the data from the primitive variables tag to a |
There was a problem hiding this comment.
I think "primitive variables tag" -> "primitive variables" is sufficient
| * \f$\Phi\f$ on the subcells so they can be used for reconstruction. | ||
| * | ||
| * The computation copies the data from the primitive variables tag to a | ||
| * new Variables and computes \f$W v^i\f$. In the future we will likely want |
There was a problem hiding this comment.
Can you clarify how "W v^i" is related to the "u_i" you mention above? At first glance these things seem inconsistent... is the "u_i" above wrong? 😛
There was a problem hiding this comment.
The brief should read W v^i (which is \gamma^{ij} u_j). I'll update the brief :)
|
|
||
| /*! | ||
| * \brief Projects the rest mass density \f$\rho\f$, pressure \f$p\f$, | ||
| * \f$u_i\f$, magnetic field \f$B^i\f$, and the divergence cleaning field |
There was a problem hiding this comment.
same question about "u_i" as in first commit
| * subcell reconstruction. | ||
| * | ||
| * The computation copies the data from the primitive variables tag to a | ||
| * new Variables and computes \f$u_i=W v_i\f$, then does the projection. In the |
There was a problem hiding this comment.
well now you've added "W v_i" into the mix.. halp
713b043 to
a125b64
Compare
|
I rebased and squashed in updates to the documentation. Thanks for the review :) |
| * we will likely want to elide this copy but that requires support from the | ||
| * actions. We should also consider if we want to use \f$u_i\f$ as a primitive | ||
| * instead of \f$v^i\f$ to further reduce work. It might also work to | ||
| * reconstruct \f$W v^i\f$ since this also guarantees a positive Lorentz factor |
There was a problem hiding this comment.
Remaining confusion: "It might also work to reconstruct W v^i..." ... but isn't that what is being done? why phrased as a hypothetical? You also say (in the previous sentence) that u_i would be less work, and then in this sentence that v^i is cheaper to compute. This sounds either inconsistent or overly simplified.. but has me confused either way..
(Note: I do think it's great to document these avenues for future exploration! I just want the suggestions to be very clear so the person revisiting this doesn't go down the wrong path...)
There was a problem hiding this comment.
Good catch :) I've deleted the extra info since that change is already implemented :)
a125b64 to
cdcd131
Compare
kidder
left a comment
There was a problem hiding this comment.
optional change you can squash
| namespace grmhd::ValenciaDivClean::subcell { | ||
| /*! | ||
| * \brief Computes the rest mass density \f$\rho\f$, pressure \f$p\f$, | ||
| * \f$W v^i\f$, magnetic field \f$B^i\f$, and the divergence cleaning field |
There was a problem hiding this comment.
I would add "Lorentz factor times the spatial velocity" before W v^i
cdcd131 to
0a17950
Compare
|
squashed in the two wording changes |
Proposed changes
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixormajor new featureif appropriate.Further comments