Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Wrong border with current background field #2326

Merged
merged 2 commits into from
Oct 26, 2017
Merged

Wrong border with current background field #2326

merged 2 commits into from
Oct 26, 2017

Conversation

psychocoderHPC
Copy link
Member

@psychocoderHPC psychocoderHPC commented Oct 25, 2017 •
edited by ax3l
Loading

fix #943

If field background is used than the border regions will be wrong.
The field background current is operationg on the local domain regions
CORE + BORDER + GUARD. The current is than scattered to the neighboring GPUs
and will create wrong border regions.

  • operate background field current only on the region CORE + BORDER
  • fix cellwiseOperation - index passed to the user functor was only correct if CORE+GUARD + BORDER was manipulated

With this fix current smoothing is still possible. Attention FieldJ is not part of the checkpointing #2325

do not merge, there is a wrong index calculation in cellwiseOperation.hpp

CC-ing: @steindev

@psychocoderHPC
fix wrong border regions with current background field
671de08 
If field background is used than the border regions will be wrong.
The field background current is operationg on the local domain regions
`CORE + BORDER + GUARD`. The current is than scattered to the neighboring GPUs
and will create wrong border regions.

- operate backgrodun field current only on the region `CORE + BORDER`
@psychocoderHPC psychocoderHPC added bug a bug in the project's code component: core in PIConGPU (core application) labels Oct 25, 2017
@psychocoderHPC psychocoderHPC added this to the 0.4.0 / 1.0.0: Next Stable milestone Oct 25, 2017
@psychocoderHPC psychocoderHPC requested a review from ax3l October 25, 2017 08:50
@ax3l ax3l requested a review from steindev October 25, 2017 08:56
@ax3l ax3l added the affects latest release a bug that affects the latest stable release label Oct 25, 2017
@ax3l
Copy link
Member

ax3l commented Oct 25, 2017

looks good, we just need a RT validation and maybe a check with current smoothing on

@psychocoderHPC
Copy link
Member Author

@steindev Could you please post the plot from the broken background field J to this PR.

@steindev
Copy link
Member

The picture shows the hdf5 output of fields/J where the current density in the simulation is defined via FieldBackgroundJ which is set to -1 everywhere. The simulation used 8 GPUs, with 2 per direction.

The FieldBackgroundJ is added in the region between two GPUs resulting in a wrong current density.

background-current_error

PrometheusPi
PrometheusPi previously approved these changes Oct 25, 2017
View reviewed changes
Copy link
Member

@PrometheusPi PrometheusPi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@steindev
Copy link
Member

Repeated test with applied fix: works!

background-current_error_fix

steindev
steindev approved these changes Oct 25, 2017
View reviewed changes
Copy link
Member

@steindev steindev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as aspected now.

@steindev
Copy link
Member

steindev commented Oct 25, 2017 •
edited
Loading

Update: Works as expected, ALMOST. Now I ran a test with a straight conductor of one cell width. The conductor is (supposed to be) located at positions x=z=96 for all y. But it is not. The current is in the wrong cell.

In the following picture, the current should be located in the top right corner of the picture. Btw. this is just a part of my simulation volume. In total it is 192x192x192.

background-current_error_fix_error

@psychocoderHPC
fix cellwiseOpertation
3c035c1 
CellwiseOperation gives wrong cell indecies to the user functor if the
kernel is only performing operations on `CORE + BORDER`

- remove host side manipulation of `totalDomainOffset`
- remove guard cells from the index given to the user functor
@PrometheusPi
Copy link
Member

PrometheusPi commented Oct 25, 2017 •
edited
Loading

@steindev Did @psychocoderHPC fix (3c035c1af6c7c184c8e9f5261c304510054fc0af) solves your issue?

@steindev
Copy link
Member

steindev commented Oct 25, 2017 •
edited
Loading

The current density is now in the expected cells. Furthermore, the pull request fixes a bug which caused the background current to be added with every time step in the left border of the simulation volume. This caused an electric field pile up within the absorber.

background-current_error_fix_error_fix

@PrometheusPi
Copy link
Member

@steindev I am confused by the left plot. If the current is equal along y, why does it reduce towards the boarder?

@steindev
Copy link
Member

steindev commented Oct 26, 2017 via email •
edited
Loading

@ax3l ax3l changed the title fix wrong border regions with current background field Wrong border with current background field Oct 26, 2017
@ax3l
Copy link
Member

ax3l commented Oct 26, 2017

thank you both, great catch! :)

@ax3l ax3l merged commit 95a9ba1 into ComputationalRadiationPhysics:dev Oct 26, 2017
@ax3l ax3l mentioned this pull request Feb 9, 2018
Merged
1 task
@ax3l ax3l mentioned this pull request Feb 16, 2018
Merged
psychocoderHPC pushed a commit to psychocoderHPC/picongpu that referenced this pull request Aug 7, 2024
@psychocoderHPC
Squashed 'thirdParty/alpaka/' changes from 6854077d6d..106a4975f4
8431571 
106a4975f4 fix getFunctionAttributes for the SYCL backend
f36e1156af update CUDA version in CI
3f8456973e use inline for CUDA/HIP code when rdc is on, otherwise use static
8b9cc3c557 fix gh-pages jobA
89d5ce671c Ignore VI temporary files
4b7bd17493 Fix the device used by KernelExecutionFixture (ComputationalRadiationPhysics#2344)
2c386dc5e9 Make alpaka follow CMAKE_CUDA_RUNTIME_LIBRARY
2d652dd233 Add thread count to CPU blocks accelerators (ComputationalRadiationPhysics#2338)
dbc5ebe1e9 Fix complex math pow test (ComputationalRadiationPhysics#2336)
4995c5b22a Fix isValidWorkDivKernel to use the correct device
f571ce9197 Remove unnecessary include
a26cdbcd41 Re-enable the KernelNoTemplateGpu test
a9217fb780 Link libcudart even when libcurand is not used
9c8614143b Suppress GCC warning about casting a function to void*
ba169cdc52 Rewrite the getValidWorkDivForKernel tests
948eb757d4 Fix getValidWorkDivForKernel tests for the SYCL CPU backend
f6f94f13b5 Fix getValidWorkDivForKernel tests for the CUDA backend
f612f971a0 Reduce code duplications in matrixMulWithMdSpan (ComputationalRadiationPhysics#2326)
d1cc2e01c1 Add a matrix multiplication example using mdspan
536a183cce Add missing whitespace in enqueue log messages
81d4410eec Reduce code duplication in CUDA/HIP kernel launch
6fdec14904  add remove-restrict
5323600508 CI: improve script utils
01d123e605 fix missing C++20 STL for ICPX in the CI
d254bcd6a3 ctest: display only output of tests, which failed
c9b8c941af change documentation
b9ed742913 remove getValidWorkDiv itself
048ef8afca use getValidWorkDivForKernel in kernelfixture of tests
38805498f0 fix random strategies
4f175420f2 remove getValidWorkDiv first
7f08120428 CI_FILTER: ^linux_nvcc11.*
789344f019 ALPAKA_FN_HOST is not a type
4efdb9dc63 fix explicit instantiation issue
fe4106f88a CI_FILTER: ^linux_nvcc11.*gcc9
e6b4881b70 CI_FILTER: ^linux_nvcc11.*gcc9
e3e760ed9e make conv2dmdspan use kernelbundle
62efffe605 Add getValidWorkDivForKernel function and KernelBundle with tests
690da679bd Let the SYCL queue implement `ConceptCurrentThreadWaitFor`, `ConceptGetDev` and `ConceptQueue` (ComputationalRadiationPhysics#2314)
995c57b54b set alpaka_CXX_STANDARD in the job generator
6ad09baa38 remove nvcc11.0 and nvcc11.1 support (ComputationalRadiationPhysics#2310)
0775f7c066 clang-format and fix typo
18eeeb7b49 move complex declaration to internal namespace
3468d2f8ac add trait IsKernelTriviallyCopyable
3015eae06b update CI container to version 3.2
56c0e416bc Update Catch2 to v3.5.2 (ComputationalRadiationPhysics#2300)

git-subtree-dir: thirdParty/alpaka
git-subtree-split: 106a4975f48dc38cc34f6a2494a3d16774282951
psychocoderHPC pushed a commit to psychocoderHPC/picongpu that referenced this pull request Aug 9, 2024
@psychocoderHPC
Squashed 'thirdParty/alpaka/' changes from 6854077d6d..ab3092357b
54253a2 
ab3092357b Use shared CUDA libraries by default
106a4975f4 fix getFunctionAttributes for the SYCL backend
f36e1156af update CUDA version in CI
3f8456973e use inline for CUDA/HIP code when rdc is on, otherwise use static
8b9cc3c557 fix gh-pages jobA
89d5ce671c Ignore VI temporary files
4b7bd17493 Fix the device used by KernelExecutionFixture (ComputationalRadiationPhysics#2344)
2c386dc5e9 Make alpaka follow CMAKE_CUDA_RUNTIME_LIBRARY
2d652dd233 Add thread count to CPU blocks accelerators (ComputationalRadiationPhysics#2338)
dbc5ebe1e9 Fix complex math pow test (ComputationalRadiationPhysics#2336)
4995c5b22a Fix isValidWorkDivKernel to use the correct device
f571ce9197 Remove unnecessary include
a26cdbcd41 Re-enable the KernelNoTemplateGpu test
a9217fb780 Link libcudart even when libcurand is not used
9c8614143b Suppress GCC warning about casting a function to void*
ba169cdc52 Rewrite the getValidWorkDivForKernel tests
948eb757d4 Fix getValidWorkDivForKernel tests for the SYCL CPU backend
f6f94f13b5 Fix getValidWorkDivForKernel tests for the CUDA backend
f612f971a0 Reduce code duplications in matrixMulWithMdSpan (ComputationalRadiationPhysics#2326)
d1cc2e01c1 Add a matrix multiplication example using mdspan
536a183cce Add missing whitespace in enqueue log messages
81d4410eec Reduce code duplication in CUDA/HIP kernel launch
6fdec14904  add remove-restrict
5323600508 CI: improve script utils
01d123e605 fix missing C++20 STL for ICPX in the CI
d254bcd6a3 ctest: display only output of tests, which failed
c9b8c941af change documentation
b9ed742913 remove getValidWorkDiv itself
048ef8afca use getValidWorkDivForKernel in kernelfixture of tests
38805498f0 fix random strategies
4f175420f2 remove getValidWorkDiv first
7f08120428 CI_FILTER: ^linux_nvcc11.*
789344f019 ALPAKA_FN_HOST is not a type
4efdb9dc63 fix explicit instantiation issue
fe4106f88a CI_FILTER: ^linux_nvcc11.*gcc9
e6b4881b70 CI_FILTER: ^linux_nvcc11.*gcc9
e3e760ed9e make conv2dmdspan use kernelbundle
62efffe605 Add getValidWorkDivForKernel function and KernelBundle with tests
690da679bd Let the SYCL queue implement `ConceptCurrentThreadWaitFor`, `ConceptGetDev` and `ConceptQueue` (ComputationalRadiationPhysics#2314)
995c57b54b set alpaka_CXX_STANDARD in the job generator
6ad09baa38 remove nvcc11.0 and nvcc11.1 support (ComputationalRadiationPhysics#2310)
0775f7c066 clang-format and fix typo
18eeeb7b49 move complex declaration to internal namespace
3468d2f8ac add trait IsKernelTriviallyCopyable
3015eae06b update CI container to version 3.2
56c0e416bc Update Catch2 to v3.5.2 (ComputationalRadiationPhysics#2300)

git-subtree-dir: thirdParty/alpaka
git-subtree-split: ab3092357bb0b917b4cc396ce49e47f7ac1924e1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steindev steindev steindev approved these changes

@ax3l ax3l ax3l approved these changes

@PrometheusPi PrometheusPi PrometheusPi left review comments

Assignees

@ax3l ax3l

Labels
affects latest release a bug that affects the latest stable release bug a bug in the project's code component: core in PIConGPU (core application)
Projects
None yet
Milestone
0.4.0: CPU Support, Filter & Merging
Development

Successfully merging this pull request may close these issues.
4 participants
@psychocoderHPC @ax3l @steindev @PrometheusPi