Fixing the initialization of the EB data in ghost cells #2635

lgiacome · 2021-12-07T19:42:11Z

With this PR we

Fix the number of ghost cells in the AMReX and WarpX EB data
Remove the FillBoundary calls for EB data as we explicitly take care of the guard cells initialization

lgiacome · 2021-12-07T19:47:32Z

Source/WarpX.cpp

+        m_field_factory[lev] = amrex::makeEBFabFactory(Geom(lev), ba, dm,
+                                                       fs_guard,
+                                                       amrex::EBSupport::full);
+#else


@WeiqunZhang @RemiLehe makeEBFabFactory always takes a three-dimensional vector for the number of guard cells, no matter the dimensions we use to compile AMReX, while the number of components of guard_cells.ng_FieldSolver depends on the dimensions.
I couldn't find a better solution than the workaround I put in place above, but I am not sure it is ideal.

WeiqunZhang · 2021-12-07T19:51:47Z

Source/WarpX.cpp

+                                guard_cells.ng_FieldSolver[0]};
+#endif
+        m_field_factory[lev] = amrex::makeEBFabFactory(Geom(lev), ba, dm,
+                                                       fs_guard,


That {1,1,1} is not the number of ghost cells in 3 directions. It's actually for different types of data (e.g., basic like cell flag, volume type like volume fraction, and everything else). Here we want to pass the the max of fs_guard three times in {}.

We need to update makeEBFabFacotry in WarpXRegrid.cpp as well.

Alright, now everything makes sense. I fixed it, thanks a lot for looking into it.

RemiLehe · 2021-12-08T18:29:28Z

This looks good.
I think that there is a small inaccuracy remaining: could you actually revert the changes that I introduced in #2607, as part of this PR ?

The reason is that, with #2607, although the guard cells are filled, they are not 100% correct, because we fill them based on the call to getType, but getType only checks the valid cells (not the guard cells) as a result of the changes introduced by #2607.
At that point, I was relying on the FillBoundary to correct this small inacurracy.

However, with the current PR, because we allocate amrex structures with the right number of guard cells, I think that we can again call getType with the guard cell and yet avoid having an out-of-bound.

Does that make sense?

lgiacome · 2021-12-08T21:37:06Z

@RemiLehe I agree with you. I've undone #2607 and the code still works as expected :)

PhilMiller · 2021-12-09T17:46:13Z

Just to confirm, this has been tested with bounds checking enabled?

PhilMiller · 2021-12-09T22:57:34Z

This fixes Modern Electron's current observed issue
#2561 (comment)

lgiacome · 2021-12-10T12:11:50Z

@PhilMiller yes :)

@return

* C++17, CMake 3.17+ (BLAST-WarpX#2300) * C++17, CMake 3.17+ Update C++ requirements to compile with C++17 or newer. * Superbuild: C++17 in AMReX/PICSAR/openPMD-api * Summit: `cuda/11.0.3` -> `cuda/11.3.1` When compiling AMReX in C++17 on Summit, the `cuda/11.0.3` module (`nvcc 11.0.2211`) dies with: ``` ... Base/AMReX_TinyProfiler.cpp nvcc error : 'cicc' died due to signal 11 (Invalid memory reference) nvcc error : 'cicc' core dumped ``` Although this usually is a memory issue, it also appears in `-j1` compiles. * Replace AMREX_SPACEDIM: Evolve & FieldSolver (BLAST-WarpX#2642) * AMREX_SPACEDIM : Boundary Conditions * AMREX_SPACEDIM : Parallelization * Fix compilation * AMREX_SPACEDIM : Initialization * Fix Typo * space * AMREX_SPACEDIM : Particles * AMREX_SPACEDIM : Evolve and FieldSolver * C++17: structured bindings to replace "std::tie(x,y,z) = f()" (BLAST-WarpX#2644) * use structured bindings * std::ignore equivalent in structured bindings Co-authored-by: Axel Huebl <[email protected]> * Perlmutter: December Update (BLAST-WarpX#2645) Update the Perlmutter instructions for the major update from December 8th, 2021. * 1D tests for plasma acceleration (BLAST-WarpX#2593) * modify requirements.txt and add input file for 1D Python pwfa * add 1D Python plasma acceleration test to CI * picmi version * USE_PSATD=OFF for 1D * Update Examples/Physics_applications/plasma_acceleration/PICMI_inputs_plasma_acceleration_1d.py Co-authored-by: Axel Huebl <[email protected]> * Update Regression/WarpX-tests.ini Co-authored-by: Axel Huebl <[email protected]> * Cartesian1D class in pywarpx/picmi.py * requirements.txt: update picmistandard * update picmi version * requirements.txt: revert unintended changes * 1D Laser Acceleration Test * Update Examples/Physics_applications/laser_acceleration/inputs_1d Co-authored-by: Axel Huebl <[email protected]> * Update Examples/Physics_applications/plasma_acceleration/PICMI_inputs_plasma_acceleration_1d.py Co-authored-by: Axel Huebl <[email protected]> * add data_list to PICMI laser_acceleration test * increase max steps and fix bug in pywarpx/picmi.py 1DCartesian moving window direction * add data_lust to Python laser acceleration test * picmistandard update Co-authored-by: Prabhat Kumar <[email protected]> Co-authored-by: Axel Huebl <[email protected]> * CMake 3.22+: Policy CMP0127 (BLAST-WarpX#2648) Fix a warning with CMake 3.22+. We use simple syntax in cmake_dependent_option, so we are compatible with the extended syntax in CMake 3.22+: https://cmake.org/cmake/help/v3.22/policy/CMP0127.html * run_test.sh: Own virtual env (BLAST-WarpX#2653) Isolate builds locally, so we don't overwrite a developer's setup anymore. This also avoids a couple of nifty problems that can occur by mixing those envs. Originally part of BLAST-WarpX#2556 * GNUmake: Fix Python Install (force) (BLAST-WarpX#2655) Local developers and cached CI installs ddi never install `pywarpx` if and old version existed. The `--force` must be with us. * Add: Regression/requirements.txt Forgotten in BLAST-WarpX#2653 * Azure: `set -eu -o pipefail` Lol, that's not the default. We previously had `script` where it was the default. Introduced in BLAST-WarpX#2615 * GNUmake & `WarpX-test.ini`: `python` -> `python3` Consistent with all other calls to Python in tests. * Fix missing checksums1d (BLAST-WarpX#2657) * Docs: Fix missing Checksum Ref * Checksum: LaserAcceleration_1d * Checksum: Python_PlasmaAcceleration_1d * Regression/requirements.txt: openpmd-api Follow-up to 8f93e01 * Azure: pre-install `setuptools` upgrade Might fix: ``` - installing setuptools_scm using the system package manager to ensure consistency - migrating from the deprecated setup_requires mechanism to pep517/518 and using a pyproject.toml to declare build dependencies which are reliably pre-installed before running the build tools warnings.warn( TEST FAILED: /home/vsts/.local/lib/python3.8/site-packages/ does NOT support .pth files You are attempting to install a package to a directory that is not on PYTHONPATH and which Python does not read ".pth" files from. The installation directory you specified (via --install-dir, --prefix, or the distutils default setting) was: /home/vsts/.local/lib/python3.8/site-packages/ and your PYTHONPATH environment variable currently contains: '' Here are some of your options for correcting the problem: * You can choose a different installation directory, i.e., one that is on PYTHONPATH or supports .pth files * You can add the installation directory to the PYTHONPATH environment variable. (It must then also be on PYTHONPATH whenever you run Python and want to use the package(s) you are installing.) * You can set up the installation directory to support ".pth" files by using one of the approaches described here: https://setuptools.readthedocs.io/en/latest/easy_install.html#custom-installation-locations Please make the appropriate changes for your system and try again. ``` * GNUmake `installwarpx`: `mv` -> `cp` No reason to rebuild. Make will detect dependency when needed. * Python GNUmake: Remove Prefix Hacks FREEEEDOM. venv power. * Azure: Ensure latest venv installed * Python/setup.py: picmistandard==0.0.18 Forgotten in BLAST-WarpX#2593 * Fix: analysis_default_regression.py Mismatched checksum file due to crude hard-coding. * PWFA 1D: Fix output name Hard coded, undocumented convention: turns out this must be the name of the test that we define in the ini file. Logical, isn't it. Not. Follow-up to BLAST-WarpX#2593 * Docs: `python3 -m pip` & Virtual Env (BLAST-WarpX#2656) * Docs: `python3 -m pip` Use `python3 -m pip`: - works independent of PATH - always uses the right Python - is the recommended way to use `pip` * Dependencies: Python incl. venv Backported from BLAST-WarpX#2556. Follow-up to BLAST-WarpX#2653 * CMake: 3.18+ (BLAST-WarpX#2651) With the C++17 switch, we required CMake 3.17+ since that one introduced the `cuda_std_17` target compile feature. It turns out that one of the many CUDA improvements in CMake 3.18+ is also to fix that feature for good, so we bump our requirement in CMake. Since CMake is easy to install, it's easier to require a clean newer version than working around a broken old one. Spotted first by Phil on AWS instances, thx! * fix check for absolute library install path (BLAST-WarpX#2646) Co-authored-by: Hannes T <[email protected]> * use if constexpr to replace template specialization (BLAST-WarpX#2660) * fix for setting the boundary condition potentials in 1D ES simulations (BLAST-WarpX#2649) * `use_default_v_<galilean,comoving>` Only w/ Boosted Frame (BLAST-WarpX#2654) * ICC CI: Unbound Vars (`setvars.sh`) (BLAST-WarpX#2663) Ignore: ``` /opt/intel/oneapi/compiler/latest/env/vars.sh: line 236: OCL_ICD_FILENAMES: unbound variable ``` * QED openPMD Tests: Specify H5 Backend (BLAST-WarpX#2661) We default to ADIOS `.bp` if available. Thus, specify HDF5 assumption * C++17: if constexpr for templates in ShapeFactors (BLAST-WarpX#2659) * use if constexpr to replace template specialization * Rmove Interface Annotations * Replace static_assert with amrex::Abort * Add includes & authors Co-authored-by: Axel Huebl <[email protected]> * ABLASTR Library (BLAST-WarpX#2263) * [Draft] ABLASTR Library - CMake object library - include FFTW wrappers to start with * Add: MPIInitHelpers * Enable ABLASTR-only builds * Add alias WarpX::ablastr * ABLASTR: openPMD forwarding * make_third_party_includes_system: Avoid Collision * WarpX: depend on `ablastr` * Definitions: WarpX -> ablastr * CMake: Reduce build objects for ABLASTR Skip all object files that we do not use in builds. * CMake: app/shared links all object targets Our `PRIVATE` source/objects are not PUBLICly propagated themselves. * Docs: Fix Warning Logger Typo (BLAST-WarpX#2667) * Python: Add 3.10, Relax upper bound (BLAST-WarpX#2664) There are no breaking changes in Python 3.10 that affect us. Giving the version compatibility of Python and it's ABI stability, there is no need at the moment to provide an upper limit. Thus, relaxed now in general. * Fixing the initialization of the EB data in ghost cells (BLAST-WarpX#2635) * Using ng_FieldSolver ghost cells in the EB data * Removed an unused variable * Fixed makeEBFabFactory also in in WarpXRgrid.cpp * Fixed end of line whitespace * Undoing BLAST-WarpX#2607 * Add PML Support for multi-J Algorithm (BLAST-WarpX#2603) * Add PML Support for multi-J Algorithm * Add CI Test * Fix the scope of profiler for SYCL (BLAST-WarpX#2668) In main.cpp, the destructor of the profiler was called after amrex::Finalize. This caused an error in SYCL due to a device synchronization call in the dtor, because the SYCL queues in amrex had been deleted. In this commit, we limit the scope of the profiler so that its destructor is called before the queues are deleted. Note that it was never an issue for CUDA/HIP, because the device synchronization calls in those backends do not need any amrex objects. * Add high energy asymptotic fit for Proton-Boron total cross section (BLAST-WarpX#2408) * Add high energy asymptotic fit for Proton Boron total cross section * Write keV and MeV instead of kev and mev * Add @return doxystrings * Add anisotropic mesh refinement example (BLAST-WarpX#2650) * Add anisotropic mesh refinement example * Update benchmark * AMReX/PICSAR: Weekly Update (BLAST-WarpX#2666) * AMReX: Weekly Update * Reset: PEC_particle, RepellingParticles, subcyclingMR New AMReX grid layout routines split grids until they truly reach number of MPI ranks, if blocking factor allows. This changes some of our particle orders slightly. * Add load balancing test (BLAST-WarpX#2561) * Added embedded_circle test * Add embedded_circle test files * Removed diag files * removed PICMI input file * Update to use default regression analysis * Added line breaks for spacing Co-authored-by: Axel Huebl <[email protected]> * Added description * Fixed benchmark file * Added load balancing to test * Commented out load_balancing portion of test. This will be added back in once load balancing is fixed. * Add load balancing to embedded_boundary test * Updated checksum * Added embedded_circle test * Add embedded_circle test files * removed PICMI input file * Update to use default regression analysis * Added load balancing to test * Commented out load_balancing portion of test. This will be added back in once load balancing is fixed. * Add load balancing to embedded_boundary test * added analysis.py file in order to relax tolerance on test * Ensure that timers are used to update load balancing algorithm * Updated test name retrieval Co-authored-by: Axel Huebl <[email protected]> Co-authored-by: Roelof <[email protected]> Co-authored-by: Roelof Groenewald <[email protected]> * Adding EB multifabs to the Python interface (BLAST-WarpX#2647) * Adding edge_lengths and face_areas to the Python interface * Added wrappers for the two new arrays of data * Adding a CI test * Fixed test name * Added customRunCmd * Added mpi in test * Refactor DepositCharge so it can be called from ImpactX. (BLAST-WarpX#2652) * Refactor DepositCharge so it can be called from ImpactX. * change thread_num * Fix namespace * remove all static WarpX:: members and methods from DepositChargeDoIt. * fix unused * Don't access ref_ratio unless lev != depos_lev * more unused * remove function to its own file / namespace * don't need a CMakeLists.txt for this * lower case namespace, rename file * Refactor: Profiler Wrapper Explicit control for synchronization instead of global state. Co-authored-by: Axel Huebl <[email protected]> * ABLASTR: Fix Doxygen in `DepositCharge` * update version number and changelog Co-authored-by: Axel Huebl <[email protected]> Co-authored-by: Prabhat Kumar <[email protected]> Co-authored-by: Luca Fedeli <[email protected]> Co-authored-by: Prabhat Kumar <[email protected]> Co-authored-by: s9105947 <[email protected]> Co-authored-by: Hannes T <[email protected]> Co-authored-by: Edoardo Zoni <[email protected]> Co-authored-by: Phil Miller <[email protected]> Co-authored-by: Lorenzo Giacomel <[email protected]> Co-authored-by: Weiqun Zhang <[email protected]> Co-authored-by: Neïl Zaim <[email protected]> Co-authored-by: Remi Lehe <[email protected]> Co-authored-by: Kevin Z. Zhu <[email protected]> Co-authored-by: Andrew Myers <[email protected]>

…2635) * Using ng_FieldSolver ghost cells in the EB data * Removed an unused variable * Fixed makeEBFabFactory also in in WarpXRgrid.cpp * Fixed end of line whitespace * Undoing BLAST-WarpX#2607

Using ng_FieldSolver ghost cells in the EB data

f0aef18

lgiacome requested review from RemiLehe and WeiqunZhang December 7, 2021 19:42

lgiacome commented Dec 7, 2021

View reviewed changes

lgiacome added bug: affects latest release Bug also exists in latest release version component: boundary PML, embedded boundaries, et al. labels Dec 7, 2021

WeiqunZhang reviewed Dec 7, 2021

View reviewed changes

lgiacome added 3 commits December 7, 2021 20:54

Removed an unused variable

a6424bb

Fixed makeEBFabFactory also in in WarpXRgrid.cpp

8aa1ce0

Fixed end of line whitespace

683104b

lgiacome changed the title ~~Fixing the initialization of ghost cells in the EB data~~ Fixing the initialization of the EB data in ghost cells Dec 7, 2021

RemiLehe self-assigned this Dec 8, 2021

Undoing BLAST-WarpX#2607

00f6c04

PhilMiller mentioned this pull request Dec 9, 2021

Add load balancing test #2561

Merged

RemiLehe approved these changes Dec 13, 2021

View reviewed changes

RemiLehe merged commit 9001fe9 into BLAST-WarpX:development Dec 13, 2021

lgiacome deleted the fix_guard_eb branch December 14, 2021 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixing the initialization of the EB data in ghost cells #2635

Fixing the initialization of the EB data in ghost cells #2635

Uh oh!

lgiacome commented Dec 7, 2021

Uh oh!

lgiacome Dec 7, 2021

Uh oh!

WeiqunZhang Dec 7, 2021

Uh oh!

WeiqunZhang Dec 7, 2021

Uh oh!

lgiacome Dec 7, 2021

Uh oh!

RemiLehe commented Dec 8, 2021

Uh oh!

lgiacome commented Dec 8, 2021

Uh oh!

PhilMiller commented Dec 9, 2021

Uh oh!

PhilMiller commented Dec 9, 2021

Uh oh!

lgiacome commented Dec 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fixing the initialization of the EB data in ghost cells #2635

Fixing the initialization of the EB data in ghost cells #2635

Uh oh!

Conversation

lgiacome commented Dec 7, 2021

Uh oh!

lgiacome Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

WeiqunZhang Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

WeiqunZhang Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

lgiacome Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

RemiLehe commented Dec 8, 2021

Uh oh!

lgiacome commented Dec 8, 2021

Uh oh!

PhilMiller commented Dec 9, 2021

Uh oh!

PhilMiller commented Dec 9, 2021

Uh oh!

lgiacome commented Dec 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants