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.

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

Fix Issue 2820 #2827

Merged
merged 6 commits into from
Jul 27, 2021
Merged

Fix Issue 2820 #2827

merged 6 commits into from
Jul 27, 2021

Conversation

shaomeng
Copy link
Collaborator

this PR fixes issue 2820 by implementing another termination condition when doing time-varying advection.

@clyne clyne requested review from StasJ and clyne July 26, 2021 14:26
@clyne
Copy link
Collaborator

clyne commented Jul 26, 2021

@shaomeng please resolve clang format issues

Copy link
Collaborator

@StasJ StasJ left a comment

Choose a reason for hiding this comment

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

This appears to fix the hanging issue by stopping if the number of steps exceeds 10000.

  1. What happens if the simulation requires > 10000 steps?
  2. In the case that it hangs, how long would the user have to wait for it to get to 10000 steps before it stops?
  3. Doesn't the bug still exist and it would just give incorrect results instead of hanging?

@shaomeng
Copy link
Collaborator Author

This appears to fix the hanging issue by stopping if the number of steps exceeds 10000.

1. What happens if the simulation requires > 10000 steps?

2. In the case that it hangs, how long would the user have to wait for it to get to 10000 steps before it stops?

3. Doesn't the bug still exist and it would just give incorrect results instead of hanging?

Good questions, so here's some clarification.

This particular bug (#2820) is caused by one particle that moves so slowly that would require a huge number of steps to advance to the targeting time step. So, what appears to be hanging is actually one particle moving slowly. If you give it enough time, it will reach the target.

Then it comes to how to resolve it. I'd think super slow moving particles are hard to avoid 100%, so setting up a cut off limit that would not affect the vast majority of particles could be a reasonable solution. Note that in this reported bug, the velocity field variables have nothing to to with actual velocities, so super slow moving particles are probably easier to occur.

Finally, the termination scheme is set up so that advection needs to proceed at least 10,000 steps or 10X the previous max num of steps. E.g., if a previous "normal" particle went for 5,000 steps, then this problematic particle will go up to 50,000 steps. I think this is a reasonable approach. The caveat is that particles that would require 55,000 or 60,000 steps to finish are terminated prematurely. However, I hope the threshold value (10,000 to start with) is set so that not many particles are affected.

@clyne clyne merged commit 8ba376e into main Jul 27, 2021
@clyne clyne deleted the issue_2820 branch July 27, 2021 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants