Skip to content

Conversation

@franzpoeschel
Copy link
Contributor

Follow-up to #5508, that PR used a needless precomputed intermediate buffer remap. Not included in that PR to avoid last-minute changes/errors.

Copy link
Member

@psychocoderHPC psychocoderHPC left a comment

Choose a reason for hiding this comment

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

Only reformating of the comments is required

@psychocoderHPC psychocoderHPC added the refactoring code change to improve performance or to unify a concept but does not change public API label Jan 5, 2026
PrometheusPi
PrometheusPi previously approved these changes Jan 5, 2026
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 fine to me - but please follow @psychocoderHPC suggestions regarding moving comments

{
continue;
}
dataPtr[remap[particleIndex]] = dataPtr[particleIndex];
Copy link
Member

Choose a reason for hiding this comment

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

This looks like magic to me and a find the new version much clearer.

Copy link
Contributor Author

@franzpoeschel franzpoeschel Jan 5, 2026

Choose a reason for hiding this comment

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

The major change is that the previous version (unnecessarily) precomputed the target indices. Could have been made a bit clearer by:

auto previouslyComputedNewIndex = remap[particleIndex];
dataPtr[previouslyComputedNewIndex] = dataPtr[particleIndex];

@PrometheusPi PrometheusPi dismissed psychocoderHPC’s stale review January 6, 2026 05:53

all requests have been fullfiled

@PrometheusPi PrometheusPi merged commit 450b8b3 into ComputationalRadiationPhysics:dev Jan 6, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring code change to improve performance or to unify a concept but does not change public API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants