-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modify stair-case approximation to the EB #5534
Modify stair-case approximation to the EB #5534
Conversation
3a60ef0
to
3181d45
Compare
…to move_stair_case_approx
This reverts commit 4fd4dc7.
@@ -174,6 +174,14 @@ WarpX::RemakeLevel (int lev, Real /*time*/, const BoxArray& ba, const Distributi | |||
using ablastr::fields::Direction; | |||
using warpx::fields::FieldType; | |||
|
|||
const auto RemakeMultiFab = [&](auto& mf){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reintroduces a function that we removed when introducing the MultiFabRegister
(#5230).
It could be removed once we introduce an iMultiFabRegister
.
|
||
void | ||
WarpX::MarkCells () | ||
WarpX::MarkExtensionCells () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function was renamed to avoid confusion with the other Mark...
function.
#endif | ||
|
||
// Skip field push in the embedded boundaries | ||
if (update_Ey_arr && update_Ey_arr(i, j, k) == 0) { return; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if
condition here is now more simple, since the logic that determines whether a cell should be updated has been moved to the function that initializes the update_E
flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I was wondering, though, did you by any chance try running the CI tests using the new m_eb_update_E
functionality but before moving the staircase approximation? That should be a really clean way to check that there are no hard to see bugs introduced in this PR, since all the CI tests should then pass with their original checksum values.
lev, PatchType::fine, | ||
warpx.GetEBUpdateEFlag()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a slight problem in that there will be an inconsistency in the hybrid solver until the follow-up PR is merged that also updates the field update for the hybrid solver. I guess we should just be sure to have both PRs merge in quick succession.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
I finalized the hybrid PR #5558 and the tests pass on that PR, so it is ready to be merged soon after this one.
@@ -1155,7 +1157,7 @@ WarpX::MacroscopicEvolveE (int lev, PatchType patch_type, amrex::Real a_dt, amre | |||
m_fields.get_alldirs(FieldType::Efield_fp, lev), | |||
m_fields.get_alldirs(FieldType::Bfield_fp, lev), | |||
m_fields.get_alldirs(FieldType::current_fp, lev), | |||
m_fields.get_alldirs(FieldType::edge_lengths, lev), | |||
m_eb_update_E[lev], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming the plan is to replace these arguments once iMultiFabRegister
is introduced?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree.
@@ -976,23 +976,21 @@ WarpX::InitLevelData (int lev, Real /*time*/) | |||
// The default maxlevel_extEMfield_init value is the total number of levels in the simulation | |||
if ((m_p_ext_field_params->B_ext_grid_type == ExternalFieldType::parse_ext_grid_function) | |||
&& (lev > 0) && (lev <= maxlevel_extEMfield_init)) { | |||
|
|||
// TODO: raise error when EB is activated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this comment here? Why should an error be raised when EB is activated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because I realized that the code is probably incorrect when using MR with the EM solver (i.e. when we use the cp
version of the fields).
This is because we should also have cp
and fp
versions of the arrays eb_update_E
/ eb_update_B
.
That being said, the problem already existed before this PR. (i.e. we should have had cp
and fp
version of edge_length
and face_area
).
For now, I cleaned up this comment and will instead open an issue with more explanation.
#ifdef AMREX_USE_EB | ||
#ifdef WARPX_DIM_3D | ||
if(lx && ((topology=='e' and lx(i, j, k)<=0) or (topology=='f' and Sx(i, j, k)<=0))) { return; } | ||
#elif defined(WARPX_DIM_XZ) || defined(WARPX_DIM_RZ) | ||
//In XZ and RZ Ex is associated with a x-edge, while Bx is associated with a z-edge | ||
if(lx && ((topology=='e' and lx(i, j, k)<=0) or (topology=='f' and lz(i, j, k)<=0))) { return; } | ||
#endif | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great simplification! 🎉
bc10261
to
16ec9b2
Compare
That's a good point! I have done this in this PR: #5574
The checksum tests indeed pass for #5574. |
When updating `E` (and when initializing the field values for `E` and `B` with a parser), we used to check somewhat complicated conditions that relied on the MultiFabs `edge_lengths` and `face_areas`. This PR introduces separate arrays (`m_eb_update_E` and `m_eb_update_B`, which are `iMultiFab` instead of `MultiFab` and thus take up much less memory). These arrays contain flags that keep track of where to udpate the fields (i.e. the black crosses in the above figure). These arrays are initialized in separate functions which keep the same EB definition. (PR #5534 will then change this definition.) The code for the field pusher and field initialization uses these arrays (instead of edge_lengths and face_areas. It is thus significantly simpler and avoids duplicating complicated if conditions. The above changes have not yet been implemented for the hybrid solver. This will instead be done in a separate PR: #5558. Once this is complete, the MultiFab `edge_lengths` and `face_areas` will not be needed anymore (except for the ECT solver), and we will thus only allocate them for the ECT solver. This should save a significant amount of memory. --------- Co-authored-by: Roelof Groenewald <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks!
Merge #5534 first. This extends the changes of #5534 and #5574 to the hybrid solver. Note that this effectively changes the definition of the stair-cased embedded boundary for the hybrid solver. --------- Co-authored-by: Roelof Groenewald <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Overview
This PR changes the definition of the stair-case approximation of the EB (used in the Yee solver), so that the actual EB boundary (where e.g. particles are removed from the simulation) is inside the stair-case-approximated boundary - as opposed to the definition used in the current
development
branch, for which the actual EB is outside the stair-case-approximated boundary.This ensures that the algorithm remains charge conserving, when charged particles are absorbed or emitted by the embedded boundary, for particle shape of order 1. (Higher-order particle shapes will be addressed in #5209.) As illustrated in the figure below (and as discussed in this paper), this is fundamentally because the particle does not deposit any charge in the valid cells, at the time when it is removed/emitted.
(The black crosses show the locations where the electric field is not updated, and thus usually remains equal to 0. The red dots show the locations where the particle deposits charge, for particle shape of order 1.)
The better behavior with respect to charge-conservation can be observed in the following animations, which show two particles of opposing charge separating and going into the embedded boundary. (Note that a static error in
divE
remains at the position where the particle was absorbed, with thedevelopment
branch. The propagating errors indivE
are expected: they are due to electromagnetic waves reflecting on the EB.)development branch
this PR
Input script: inputs.txt
Analysis script: openPMD-visualization.ipynb.txt
(An automated tests using a similar configuration has been added in a separate, follow-up PR: #5562)
Note that, as part of this PR, the above new definition has been adopted for all the finite-difference solvers, except for the ECT solver (which uses a cut-cell representation instead of a stair-case representation).
Implementation
This PR uses the changes of #5574. It still uses
MarkUpdateECellsECT
andMarkUpdateBCellsECT
for the ECT sover - which preserve the previous behavior of the embedded boundary for this solver, but now usesMarkUpdateCellsStairCase
for the other FDTD solvers - which introduce the above-mentioned new stair-case definition.