Skip to content

Fixissue/#415 - #416

Open
aaadelmann wants to merge 21 commits into
masterfrom
fixissue/#415
Open

Fixissue/#415#416
aaadelmann wants to merge 21 commits into
masterfrom
fixissue/#415

Conversation

@aaadelmann

@aaadelmann aaadelmann commented Sep 24, 2025

Copy link
Copy Markdown
Member
## Summary

This PR add mixed-value scatter and fixes CUDA compilation issues.

The affected API is `ParticleAttrib::scatter`, in particular cases where:
- a custom `Kokkos::RangePolicy` is used,
- an optional `hash_type` index map is supplied,
- the particle attribute value type differs from the field value type, for example `ParticleAttrib<float> -> Field<double>`.

## What Changed

### CUDA-safe scatter dispatch

`ParticleAttrib::scatter` now performs the hash/no-hash choice on the host and dispatches to a compile-time implementation:

```cpp
particleAttribScatterImpl<true>(...)
particleAttribScatterImpl<false>(...)

The actual Kokkos kernel body was moved out of the private ParticleAttrib member scope into a namespace-scope helper:

ippl::detail::particleAttribScatterImpl(...)

This avoids NVCC errors for extended __host__ __device__ lambdas whose enclosing parent function has private/protected class access.

The mapped-index selection is also handled through a small KOKKOS_INLINE_FUNCTION helper:

ippl::detail::scatterMappedIndex<UseHashView>(idx, hashView)

This avoids CUDA’s restriction around first-capturing a variable inside an if constexpr context.

Mixed value type scatter

The scatter implementation now supports different particle attribute and field value types, for example:

ParticleAttrib<float> q;
Field<double, Dim, Mesh, Centering> rho;

scatter(q, rho, positions);

The scattered particle value is passed to the field scatter operation using the particle attribute’s value type, while the field accumulation happens in the field’s value type.

Regression tests

unit_tests/Particle/GatherScatterTest.cpp now includes explicit coverage for:

  • ParticleAttrib<float> -> Field<double> plain scatter
  • ParticleAttrib<float> -> Field<double> hashed scatter
  • dimensions 1, 2, and 3 through the existing typed test suite

The test uses a conservation tolerance based on the weaker precision of the scatter attribute and field value type. This avoids applying double-precision tolerances to a float-valued particle source while still scaling the tolerance with the conserved charge norm.

Motivation

The original hashed scatter refactor triggered CUDA compilation failures on GH200/NVCC:

error: An extended __host__ __device__ lambda cannot first-capture variable in constexpr-if context

and then:

error: The enclosing parent function ("scatterImpl") for an extended __host__ __device__ lambda cannot have private or protected access within its class

These were CUDA-specific restrictions around extended lambdas. The final implementation keeps the same host-side dispatch semantics, but places the kernel-bearing function in namespace scope and avoids first-capture inside if constexpr.

Validation

Tested locally with a serial debug build:

cmake --build build-fixissue-415-merge --target GatherScatterTest -j 8
ctest --test-dir build-fixissue-415-merge --output-on-failure -R '^GatherScatterTest$'
mpirun -np 2 build-fixissue-415-merge/unit_tests/Particle/GatherScatterTest --gtest_filter='*MixedValueType*'
mpirun -np 4 build-fixissue-415-merge/unit_tests/Particle/GatherScatterTest --gtest_filter='*MixedValueType*'

All tests passed.

The CUDA build was validated externally through the project CI/CD matrix after the scatter implementation changes.

@aaadelmann

Copy link
Copy Markdown
Member Author

closes #415

@srikrrish srikrrish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me. But currently there is one conflict in the CMakeLists which must be resolved and CI needs to be invoked before merging

@aaadelmann
aaadelmann marked this pull request as draft July 28, 2026 14:32
Resolve the ParticleAttrib scatter conflict by keeping master's captured local view form while preserving the mixed attribute/field type behavior covered by the branch tests.

Preserve the cosmology mc-4 initializer after master moved demos under demos/cosmology by moving the initializer sources there and wiring them into the StructureFormation target.
@aaadelmann aaadelmann self-assigned this Aug 6, 2026
@aaadelmann aaadelmann added the enhancement New feature or request label Aug 6, 2026
aaadelmann and others added 6 commits August 6, 2026 16:30
Split ParticleAttrib::scatter into a host-side hash/no-hash dispatcher and a
templated scatterImpl so the non-hashed kernel does not instantiate hash-array
indexing.

Route the mapped-index selection through a small KOKKOS_INLINE_FUNCTION helper.
This avoids the CUDA extended-lambda restriction where a variable is first
captured inside an if constexpr context, while still keeping the hash access
compiled out for the non-hashed instantiation.

Also normalize the hash extent to the range-policy end type before checking the
policy range, removing the signed/unsigned comparison warning.
Move the hash-aware ParticleAttrib scatter kernel body out of the
ParticleAttrib private member scope and into a namespace-scope detail helper.

CUDA extended host-device lambdas cannot be enclosed by private or protected
class member functions. The previous scatterImpl helper fixed the hash-array
constexpr dispatch but still placed the KOKKOS_LAMBDA inside a private member,
which NVCC rejects for CUDA builds.

Keep ParticleAttrib::scatter as the public host-side dispatcher. It validates
the optional hash view and selects either the hashed or non-hashed
compile-time implementation:

  particleAttribScatterImpl<true>
  particleAttribScatterImpl<false>

The mapped-index selection remains in a KOKKOS_INLINE_FUNCTION helper so the
hash access is compiled only for the hashed instantiation, avoiding CUDA's
first-capture-in-constexpr-if extended-lambda restriction.

Tested locally with a SERIAL debug build:

  cmake --build build-fixissue-415-merge -j 8
  ctest --test-dir build-fixissue-415-merge --output-on-failure -R '^(TestScatter|GatherScatterTest)$'

Both GatherScatterTest and TestScatter passed.
Add explicit GatherScatterTest coverage for scattering a lower-precision
particle attribute into a higher-precision field:

  ParticleAttrib<float> -> Field<double>

This exercises the scatter API change that allows the particle attribute value
type to differ from the field value type. The new coverage includes both the
plain scatter path and the custom hash-array scatter path, so the host-side
hash/no-hash dispatch and the CUDA-safe scatter implementation are both covered
for mixed value types.

Extend the test bunch with an additional float charge attribute and add a
generic hash-reduction helper so expected charges can be accumulated in double
while reading either float or double particle attributes.

Add a scatterConservationTolerance helper that chooses the tolerance from the
weaker precision of the scatter attribute and field value type. This avoids
testing float -> double scatter with an unrealistically strict double tolerance
while still scaling the allowed error with the conserved charge norm.

Tested locally with:

  cmake --build build-fixissue-415-merge --target GatherScatterTest -j 8
  ctest --test-dir build-fixissue-415-merge --output-on-failure -R '^GatherScatterTest$'
  mpirun -np 2 build-fixissue-415-merge/unit_tests/Particle/GatherScatterTest --gtest_filter='*MixedValueType*'
  mpirun -np 4 build-fixissue-415-merge/unit_tests/Particle/GatherScatterTest --gtest_filter='*MixedValueType*'

All tests passed.
@aaadelmann
aaadelmann marked this pull request as ready for review August 6, 2026 17:36
@aaadelmann
aaadelmann requested a review from srikrrish August 6, 2026 17:37
@aaadelmann

Copy link
Copy Markdown
Member Author

You already approved in the past, here I needed to fix some CUDA related issues and added an other test

@aaadelmann

Copy link
Copy Markdown
Member Author

cscs-ci run cscs-ci-gh200, cscs-ci-mi300, cscs-ci-openmp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request gitlab-mirror

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants