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

tol parameter for trace_particle_through_mesh #141

Closed
wants to merge 3 commits into from

Conversation

Fuad-HH
Copy link
Contributor

@Fuad-HH Fuad-HH commented Feb 11, 2025

Previously tolerance for the search method, trace_particle_through_mesh, was computed every time it is called which can be unnecessary if the mesh doesn't change as described in issue #138. It involves a parallel reduction operation which can be expensive.

Closes #138.

- passing all tests except smoke_test_particle
It is giving a warning for MPI oversubscribing
and "Structure not initalized at Particle"
- passing all tests
Copy link
Contributor

@cwsmith cwsmith 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. I left a comment below about passing tol.

@@ -466,7 +467,8 @@ namespace pumipic {
o::Write<o::Real>& inter_points,
int looplimit,
bool debug,
Func& func) {
Func& func,
o::Real &tol) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can tol be made an optional argument to avoid the 'checking for negative' logic? Something like this:
https://godbolt.org/z/Poa5haq6d

instead of checking if tol was passed using negative values,
it is made optional. In the future, tol will be calculated
outside according to the standardization plan and for this
reason, we don't need it to be passed as reference or pointer
@cwsmith
Copy link
Contributor

cwsmith commented Feb 12, 2025

@Fuad-HH Given how similar this PR is to #142, I think they should be combined. Is there a reason not to combine them?

@Fuad-HH
Copy link
Contributor Author

Fuad-HH commented Feb 12, 2025

Yes, we can combine them. Does this mean to delete this pull request #141 since the new one #142 already includes it?

@cwsmith
Copy link
Contributor

cwsmith commented Feb 12, 2025

Yes. Closing this one makes sense.

@Fuad-HH Fuad-HH closed this Feb 12, 2025
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.

Pass Tolerance to trace_particle_through_mesh Instead of Computing it Everytime
2 participants