PML Exchanges: Less Duplicate Code #2394

EZoni · 2021-10-08T00:08:42Z

Similarly to #2375, I'm trying to see if we can reduce the amount of duplicate code, in this case for the functions Exchange<E,B> of the PML class. Open for discussion.

EZoni

Just adding a couple of questions as review comments.

EZoni · 2021-10-08T00:14:17Z

Source/BoundaryConditions/PML.H

+    const amrex::Geometry* m_geom;
+    const amrex::Geometry* m_cgeom;


@ax3l @WeiqunZhang I made these two public because I would need to pass them as arguments when I call Exchange from a PML object elsewhere (see changes in Source/Parallelization/WarpXComm.cpp). Do you think I should define public "getters" instead? (I guess I'm a little undecided because these are pointers themselves ...)

Another option could be to pass a boolean flag is_coarse or similar to the PML exchange method. That way you don't have to pass a member variable as an argument to a member function of the same class (the same could be done for the mf_pml by the way, using an enum struct argument).
But honestly, I wouldn't know which solution would be considered best practice.

More generally, your PR looks good, thanks!

@NeilZaim Thanks for your comment. I think what you mention was the purpose of the original argument patch_type (of type PatchType instead of simply bool), which was inspected in the old functions ExchangeE and ExchangeB. However, the reason why I removed that argument and its inspection is that I think those checks were redundant, in some sense. In particular, the logic of those function calls were the following (for example for the fine patch):

// Here we are in WarpX::FillBoundaryE or WarpX::FillBoundaryB // in Source/Parallelization/WarpXComm.cpp if (patch_type == PatchType::fine) { if (do_pml && pml[lev]->ok()) { // call ExchangeE or ExchangeB from a PML object, // passing patch_type, which is inspected *again* with // if (patch_type == PatchType::fine && ... ) // inside those functions } }

I removed patch_type as an argument passed to the new function Exchange because in general I don't think there are good reasons for us to check if patch_type == PatchType::fine twice.

If the double if check is the main issue, you could consider moving the if (do_pml && pml[lev]->ok()) block outside of the first if (patch_type == PatchType::fine) and then pass patch_type as an argument and do something like const auto geom = (patch_type == PatchType::fine) ? m_geom : m_cgeom;.

But as I said above, this will not make a difference in terms of either performance or code duplication, so I really don't know what's considered better (it just felt a bit weird to pass a member of class PML as argument to a function of class PML).

I think we cannot move those calls outside the if (patch_type == PatchType::fine) because based on the result of that if condition we also choose to get the fp or cp pointer for E and B respectively. So this would require the refactoring of the similar functions at the level of Parallelization/WarpXComm.cpp, too. I could do that here, or maybe in another separate PR, to keep things simple. Anyways I agree with you that passing the Geometry members to a PML function does not make much sense, so I removed that. We still remove code duplication in the PML, even if we leave the double check on the patch type for now.

With regard to this thread, I removed some more code duplication in the latest commits. Will leave the rest, if needed and if possible, to future PRs.

EZoni · 2021-10-08T00:17:28Z

Source/BoundaryConditions/PML.cpp

+    if (mf_pml[0] && mf[0]) Exchange(*mf_pml[0], *mf[0], geom, do_pml_in_domain);
+    if (mf_pml[1] && mf[1]) Exchange(*mf_pml[1], *mf[1], geom, do_pml_in_domain);
+    if (mf_pml[2] && mf[2]) Exchange(*mf_pml[2], *mf[2], geom, do_pml_in_domain);


Isn't it maybe a little redundant to perform these if checks here at this level? These functions are called elsewhere in Source/Parallelization/WarpXComm.cpp after conditional checks such as if (do_pml && pml[lev]->ok()) and similar. I wonder if we should perform more safety checks in the constructor of the PML class and less safety checks in functions like this one here. What do you think?

EZoni · 2021-11-17T15:55:56Z

Note: #2211 should be merged first to avoid conflicts. This PR should then be rebased and merged afterwards (hence the "[WIP]" flag). In the meantime, though, I will request another round of review.

NeilZaim

Hi @EZoni. Sorry for talking a long time to review. This PR looks good to me, I've just left some minor suggestions about const correctness.

NeilZaim · 2021-12-01T08:30:00Z

Source/Parallelization/WarpXComm.cpp

@@ -544,57 +544,42 @@ WarpX::FillBoundaryE(int lev, IntVect ng)
 }

 void
-WarpX::FillBoundaryE (int lev, PatchType patch_type, IntVect ng)
+WarpX::FillBoundaryE (const int lev, PatchType patch_type, amrex::IntVect ng)


Suggested change

WarpX::FillBoundaryE (const int lev, PatchType patch_type, amrex::IntVect ng)

WarpX::FillBoundaryE (const int lev, const PatchType patch_type, const amrex::IntVect ng)

NeilZaim · 2021-12-01T08:30:34Z

Source/Parallelization/WarpXComm.cpp

-            WarpXCommUtil::FillBoundary(*Efield_cp[lev][1], ng, cperiod);
-            WarpXCommUtil::FillBoundary(*Efield_cp[lev][2], ng, cperiod);
-        }
+        amrex::IntVect nghost = (safe_guard_cells) ? mf[i]->nGrowVect() : ng;


Suggested change

amrex::IntVect nghost = (safe_guard_cells) ? mf[i]->nGrowVect() : ng;

const amrex::IntVect nghost = (safe_guard_cells) ? mf[i]->nGrowVect() : ng;

NeilZaim · 2021-12-01T08:30:49Z

Source/Parallelization/WarpXComm.cpp

@@ -606,57 +591,42 @@ WarpX::FillBoundaryB (int lev, IntVect ng)
 }

 void
-WarpX::FillBoundaryB (int lev, PatchType patch_type, IntVect ng)
+WarpX::FillBoundaryB (const int lev, PatchType patch_type, amrex::IntVect ng)


Suggested change

WarpX::FillBoundaryB (const int lev, PatchType patch_type, amrex::IntVect ng)

WarpX::FillBoundaryB (const int lev, const PatchType patch_type, const amrex::IntVect ng)

NeilZaim · 2021-12-01T08:31:02Z

Source/Parallelization/WarpXComm.cpp

+            ng <= mf[i]->nGrowVect(),
+            "Error: in FillBoundaryB, requested more guard cells than allocated");
+
+        amrex::IntVect nghost = (safe_guard_cells) ? mf[i]->nGrowVect() : ng;


Suggested change

amrex::IntVect nghost = (safe_guard_cells) ? mf[i]->nGrowVect() : ng;

const amrex::IntVect nghost = (safe_guard_cells) ? mf[i]->nGrowVect() : ng;

NeilZaim · 2021-12-01T08:31:36Z

Source/WarpX.H

+    void FillBoundaryB (const int lev, PatchType patch_type, amrex::IntVect ng);
+    void FillBoundaryE (const int lev, PatchType patch_type, amrex::IntVect ng);


Suggested change

void FillBoundaryB (const int lev, PatchType patch_type, amrex::IntVect ng);

void FillBoundaryE (const int lev, PatchType patch_type, amrex::IntVect ng);

void FillBoundaryB (const int lev, const PatchType patch_type, const amrex::IntVect ng);

void FillBoundaryE (const int lev, const PatchType patch_type, const amrex::IntVect ng);

EZoni · 2022-01-20T01:16:14Z

@WeiqunZhang What is your opinion on the changes made in this PR?

* remove check on geometry (ECP-WarpX#2771) * RZ: Add error message when using 0 order for azimuthal decomposition (ECP-WarpX#2772) * CI: GNUmake (ECP-WarpX#2769) Since we migrated also Azure regression tests to CMake now, we should add a GNUmake build to CI, so we don't accidentally lose this capability (we will not duplicate the whole CI matrix). Does a standard CUDA build without dependencies but enabling PSATD. * PML Exchanges: Less Duplicate Code (ECP-WarpX#2394) Similarly to ECP-WarpX#2375, I'm trying to see if we can reduce the amount of duplicate code, in this case for the functions `Exchange<E,B>` of the PML class. Open for discussion. * AMReX: Update latest (ECP-WarpX#2773) * Docs: Crusher Load `cce` module (ECP-WarpX#2770) * Docs: Crusher Load `cce` module Without that module, HIP RDC builds do not work. https://docs.olcf.ornl.gov/systems/crusher_quick_start_guide.html#compilers * cce/13.0.0 -> cce/13.0.1 * Crusher: load `cce` after `rocm` * RZ FDTD: Filter Not Working (Abort) (ECP-WarpX#2775) * RZ FDTD: Filter Not Working (Abort) * Update CI Tests and Benchmarks * Funding/Acknowledgements: CEA-LIDYL. (ECP-WarpX#2781) * Update .zenodo.json (ECP-WarpX#2782) * Add SyncRho call to warpx_depositChargeDensity (ECP-WarpX#2777) * add SyncRho call to warpx_depositChargeDensity * expose SyncRho to Python; add warpx_clearChargeDensity to reset rho_fp before deposition * remove unneeded warpx_clearChargeDensity function * Apply suggestions from code review Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja> * add (default) option to depositChargeDensity to call SyncRho after deposition * added option to depositChargeDensity to zero out rho_fp before deposition * code cleanup * import fields from inside depositChargeDensity Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja> * udpate zenodo (ECP-WarpX#2784) Co-authored-by: Hannah Klion <hannah.klion@gmail.com> * expose WarpXParticleContainer::sumParticleCharge to Python (ECP-WarpX#2790) * Add tiny profiling to reduced diagnostics (ECP-WarpX#2794) * fix typo in docstring for ImpactIonizationTransformFunc constructor (ECP-WarpX#2793) * style fixes in WarpXWrappers.cpp (ECP-WarpX#2792) * Regressions: Remove Tolerance (ECP-WarpX#2789) ... otherwise we build the `fvarnames` tools automatically. * Add Python callbacks before and after collisions (ECP-WarpX#2791) * add python callbacks before and after collisions * re-order callback declarations * Fix Docs of MinAndMaxPositions (ECP-WarpX#2787) Fix order of returned tuple (doc string). * Track injection statistics for MCC (#145) * added verbosity flag to MCC * added MCC verbosity flag to mewarpx * expose WarpXParticleContainer::sumParticleCharge to Python * add python callbacks before and after collisions * example of tracking MCC statistics * add MCC injection tracking ability via flux_diagnostic * added MCC to runinfo in diode_setup so that it is added to the flux diagnostic; also updated the thermionic_ignition.py example file to use setups_store * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * remove unneeded call to init_inert_gas in tests using setups_store/diode_setup.py * updated changelog and version number * revert MCC verbosity flag, more info is better than less * avoid MCC injection tracking if it is not used in the simulation * remove duplicated code due to merge * add appropriate date to changelog Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Luca Fedeli <luca.fedeli@cea.fr> Co-authored-by: Neïl Zaim <49716072+NeilZaim@users.noreply.github.com> Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja> Co-authored-by: Edoardo Zoni <59625522+EZoni@users.noreply.github.com> Co-authored-by: Prabhat Kumar <89051199+prkkumar@users.noreply.github.com> Co-authored-by: Hannah Klion <hannah.klion@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

EZoni added cleaning Clean code, improve readability component: parallelization Guard cell exchanges and particle redistribution component: boundary PML, embedded boundaries, et al. labels Oct 8, 2021

EZoni commented Oct 8, 2021

View reviewed changes

PML Exchanges: Less Duplicate Code

e86c971

EZoni force-pushed the Exchange_PML_lesscode branch from baae6c0 to e86c971 Compare November 15, 2021 21:53

EZoni added 4 commits November 15, 2021 14:48

Avoid Passing Geometry members to PML::Exchange

ef14f91

Simplify FillBoundaryE in WarpXComm.cpp

a78e95e

Fix Bugs

aa35cb8

Simplify FillBoundaryB in WarpXComm.cpp

35e479e

EZoni requested review from NeilZaim and WeiqunZhang November 17, 2021 15:58

Merge ECP-WarpX:development into EZoni:Exchange_PML_lesscode

7911617

NeilZaim approved these changes Dec 1, 2021

View reviewed changes

EZoni added 5 commits December 1, 2021 09:32

Improve const correctness

50972ef

Merge ECP-WarpX:development into EZoni:Exchange_PML_lesscode

9b7c3d0

Merge ECP-WarpX:development into EZoni:Exchange_PML_lesscode

efdf517

Merge ECP-WarpX:development into EZoni:Exchange_PML_lesscode

490e9ff

Merge ECP-WarpX:development into EZoni:Exchange_PML_lesscode

d1e7286

EZoni changed the title ~~[WIP] PML Exchanges: Less Duplicate Code~~ PML Exchanges: Less Duplicate Code Jan 6, 2022

Merge ECP-WarpX:development into EZoni:Exchange_PML_lesscode

664749a

ax3l assigned RemiLehe Jan 13, 2022

Merge ECP-WarpX:development into EZoni:Exchange_PML_lesscode

e6d936d

Merge ECP-WarpX:development into EZoni:Exchange_PML_lesscode

4532e69

WeiqunZhang approved these changes Jan 24, 2022

View reviewed changes

WeiqunZhang merged commit 0865f6b into ECP-WarpX:development Jan 24, 2022

EZoni deleted the Exchange_PML_lesscode branch September 19, 2024 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PML Exchanges: Less Duplicate Code #2394

PML Exchanges: Less Duplicate Code #2394

EZoni commented Oct 8, 2021 •

edited

Loading

EZoni left a comment

EZoni Oct 8, 2021

NeilZaim Oct 8, 2021

EZoni Oct 8, 2021

NeilZaim Oct 8, 2021 •

edited

Loading

EZoni Nov 15, 2021 •

edited

Loading

EZoni Nov 17, 2021 •

edited

Loading

EZoni Oct 8, 2021 •

edited

Loading

EZoni commented Nov 17, 2021 •

edited

Loading

NeilZaim left a comment

NeilZaim Dec 1, 2021

NeilZaim Dec 1, 2021

NeilZaim Dec 1, 2021

NeilZaim Dec 1, 2021

NeilZaim Dec 1, 2021

EZoni commented Jan 20, 2022

		const amrex::Geometry* m_geom;
		const amrex::Geometry* m_cgeom;

	WarpX::FillBoundaryE (const int lev, PatchType patch_type, amrex::IntVect ng)
	WarpX::FillBoundaryE (const int lev, const PatchType patch_type, const amrex::IntVect ng)

	amrex::IntVect nghost = (safe_guard_cells) ? mf[i]->nGrowVect() : ng;
	const amrex::IntVect nghost = (safe_guard_cells) ? mf[i]->nGrowVect() : ng;

	WarpX::FillBoundaryB (const int lev, PatchType patch_type, amrex::IntVect ng)
	WarpX::FillBoundaryB (const int lev, const PatchType patch_type, const amrex::IntVect ng)

		void FillBoundaryB (const int lev, PatchType patch_type, amrex::IntVect ng);
		void FillBoundaryE (const int lev, PatchType patch_type, amrex::IntVect ng);

PML Exchanges: Less Duplicate Code #2394

PML Exchanges: Less Duplicate Code #2394

Conversation

EZoni commented Oct 8, 2021 • edited Loading

EZoni 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

NeilZaim Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

EZoni Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

EZoni Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

EZoni Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

EZoni commented Nov 17, 2021 • edited Loading

NeilZaim 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

EZoni commented Jan 20, 2022

EZoni commented Oct 8, 2021 •

edited

Loading

NeilZaim Oct 8, 2021 •

edited

Loading

EZoni Nov 15, 2021 •

edited

Loading

EZoni Nov 17, 2021 •

edited

Loading

EZoni Oct 8, 2021 •

edited

Loading

EZoni commented Nov 17, 2021 •

edited

Loading