Skip to content

Leverage particle.move_distance in event advance #3544

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

Merged
pshriwise merged 1 commit into from
Aug 22, 2025
Merged

Leverage particle.move_distance in event advance #3544

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/particle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,7 @@ void Particle::event_advance()
std::min({boundary().distance(), collision_distance(), distance_cutoff});

// Advance particle in space and time
// Short-term solution until the surface source is revised and we can use
Copy link
Contributor

@pshriwise pshriwise Aug 22, 2025

Choose a reason for hiding this comment

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

To close the loop here, do we know if there is an issue or PR related to this comment we can cite in this PR that resolved the surface source problem?

Copy link
Contributor Author

@GuySten GuySten Aug 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This comment was added in #2631. You can read the discussion there but it was not clear to me what the problem was or why now it's working.

Copy link
Contributor

@pshriwise pshriwise Aug 22, 2025

Choose a reason for hiding this comment

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

Me either, but the behavior should be equivalent so I won't let it hold up this PR.

Copy link
Contributor

@pshriwise pshriwise Aug 22, 2025

Choose a reason for hiding this comment

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

Maybe the intent was to include time updates inside move_distance. @cfichtlscherer, can you provide some extra info here?

Copy link
Contributor

@cfichtlscherer cfichtlscherer Sep 7, 2025

Choose a reason for hiding this comment

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

Sorry, can't remember why we had it in a loop, but looks good for me.

Copy link
Contributor

@paulromano paulromano Sep 20, 2025

Choose a reason for hiding this comment

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

There was a surface source test that was failing when the change to use move_distance was made. It was unclear at the time why -- probably some weirdness with how the compiler was optimizing code resulting in slightly different numeric results. Whatever the reason was, there have evidently been enough changes (compilers, tests, implementation, underlying libraries) that it is no longer affected as evidenced by the tests on this PR passing.

Here was my original comment about it:
#2454 (comment)

// this->move_distance(distance)
for (int j = 0; j < n_coord(); ++j) {
coord(j).r() += distance * coord(j).u();
}
this->move_distance(distance);
double dt = distance / speed;
this->time() += dt;
this->lifetime() += dt;
Expand Down