Skip to content

clear particles in pgen - #1441

Open
Yurlungur wants to merge 15 commits into
developfrom
jmm/clear-particles-in-pgen
Open

clear particles in pgen#1441
Yurlungur wants to merge 15 commits into
developfrom
jmm/clear-particles-in-pgen

Conversation

@Yurlungur

@Yurlungur Yurlungur commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

PR Summary

In the process of going through the tutorial #1439 @pgrete pointed out that particles cannot currently be safely set in the problem generator, because they aren't cleared between remeshing, meaning there's a memory leak (and also probably undefined behavior).

This MR resolves the issue by just clearing particles before each pgen instance. I also modify the particle tracers example to stress this mechanism.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • Any contribution that was created or modified with the assistance of generative AI is disclosed here and in code following the guidelines
  • (@lanl.gov employees) Update copyright on changed files

@Yurlungur
Yurlungur requested review from pdmullen and pgrete August 24, 2026 23:07
@Yurlungur Yurlungur self-assigned this Aug 24, 2026
@Yurlungur Yurlungur added the bug Something isn't working label Aug 24, 2026
@Yurlungur
Yurlungur enabled auto-merge August 24, 2026 23:10
@pdmullen

pdmullen commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

This was the reason for the introduction of PostInitialization. Please see #1377

@Yurlungur

Copy link
Copy Markdown
Collaborator Author

Why not offer both options? This allows particles to be created in pgen safely if desired.

@pdmullen

pdmullen commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Why not offer both options? This allows particles to be created in pgen safely if desired.

Happy to support both options 😄

I am just saying that the current behavior might be the "expected behavior" (...if you will...). i.e., you classify this as a memory leak or bug, but I think another interpretation is that it is doing exactly what is expected:

You source particles in a block. The block refines. Then the Parthernon particle AMR logic (I believe) maps those correctly to daughter blocks. Then you call ProblemGenerator again and source particles on those new daughter blocks and the process continues until the mesh resolves. And in this interpretation, if that isn't the behavior a user wants, then they might instead opt for PostInitialization.

@Yurlungur

Copy link
Copy Markdown
Collaborator Author

Gotcha and point taken. Agreed this is not a bug then.

Happy to be convinced to leave things as is, not I think clearing before the pgen is still a useful thing to offer, as it prevents a foot gun for new users.

For test coverage, what about modifying the test to run in both modes?

@Yurlungur Yurlungur removed the bug Something isn't working label Aug 26, 2026
@Yurlungur

Copy link
Copy Markdown
Collaborator Author

@par-hermes format

@Yurlungur
Yurlungur disabled auto-merge August 31, 2026 17:58
@Yurlungur
Yurlungur enabled auto-merge August 31, 2026 17:58
@Yurlungur

Copy link
Copy Markdown
Collaborator Author

@pdmullen please re-review and let me know if this seems like a good compromise.
@c-prather and @pgrete please review as this is a dependnecy of the tutorial MR.

@pdmullen

Copy link
Copy Markdown
Collaborator

LGTM!

@pgrete pgrete left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Only one minor comments on testing a probably more typical path.

Apart from that, this now removes the possibility to seed particles exactly once on the root level, which I think is fine.

Comment thread example/particle_tracers/particle_tracers.cpp Outdated
Comment thread CHANGELOG.md Outdated
@Yurlungur

Copy link
Copy Markdown
Collaborator Author

@par-hermes format

@Yurlungur

Copy link
Copy Markdown
Collaborator Author

@pgrete please re-review. I think your comments should be addressed.

@pgrete pgrete left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the update.
LGTM

@pgrete
pgrete disabled auto-merge September 2, 2026 11:23
@pgrete
pgrete enabled auto-merge (squash) September 2, 2026 11:23
@Yurlungur

Copy link
Copy Markdown
Collaborator Author

CI jobs canceled for some reason... re-running.

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.

4 participants