-
Notifications
You must be signed in to change notification settings - Fork 24
Add wrappers to invalidate particles and set id/cpu from Python #435
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
Conversation
for more information, see https://pre-commit.ci
EZoni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @atmyers! I left a few minor comments, please consider if the suggestions are worth being applied. After this round of review the PR is ready to merge, as far as I'm concerned. I think it would be nice to have this feature before we release 25.05.
cmake/dependencies/AMReX.cmake
Outdated
| CACHE STRING | ||
| "Repository URI to pull and build AMReX from if(pyAMReX_amrex_internal)") | ||
| set(pyAMReX_amrex_branch "25.04" | ||
| set(pyAMReX_amrex_branch "development" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we usually set a commit hash here, rather than the name of the branch. But this works too, of course. It will be soon overwritten with 25.05 anways, when we publish the new release.
src/Particle/ParticleContainer.cpp
Outdated
|
|
||
| auto buf = idcpus.request(); | ||
| auto buf2 = cpus.request(); | ||
| if (buf.size != buf2.size) throw std::runtime_error("sizes do not match!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we enclose this in parenthesis just for consistency with the other one-line if conditions?
| if (buf.size != buf2.size) throw std::runtime_error("sizes do not match!"); | |
| if (buf.size != buf2.size) { | |
| throw std::runtime_error("sizes do not match!"); | |
| } |
src/Particle/ParticleContainer.cpp
Outdated
|
|
||
| auto buf = idcpus.request(); | ||
| auto buf2 = ids.request(); | ||
| if (buf.size != buf2.size) throw std::runtime_error("sizes do not match!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we enclose this in parenthesis just for consistency with the other one-line if conditions?
| if (buf.size != buf2.size) throw std::runtime_error("sizes do not match!"); | |
| if (buf.size != buf2.size) { | |
| throw std::runtime_error("sizes do not match!"); | |
| } |
| idcpu = np.array( | ||
| [ | ||
| 9223372036871553124, | ||
| 9223372036871553124, | ||
| 9223372036871553124, | ||
| 9223372036871553124, | ||
| 9223372036871553124, | ||
| ], | ||
| dtype=np.uint64, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could add an inline comment clarifying where this number comes from, just to make the test as clear as possible.
EZoni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
To see how this is used, see tests/test_particleTile.py