Skip to content

Conversation

@mayukh33
Copy link
Contributor

No description provided.

@mayukh33 mayukh33 requested a review from clpetix October 27, 2025 18:24
@clpetix clpetix mentioned this pull request Oct 27, 2025
@clpetix
Copy link
Contributor

clpetix commented Nov 7, 2025

@mayukh33, I've edited the example slightly. We will want to remove the trajectory file and just have users run it themselves if they want to follow along. The diff on this is massive and the file is quite large 😅

@mayukh33 mayukh33 closed this Nov 7, 2025
@mayukh33 mayukh33 reopened this Nov 7, 2025
@mayukh33
Copy link
Contributor Author

mayukh33 commented Nov 7, 2025

@mayukh33, I've edited the example slightly. We will want to remove the trajectory file and just have users run it themselves if they want to follow along. The diff on this is massive and the file is quite large 😅

That makes sense! I will remove that. Do we want to keep the python initialization script?

@clpetix
Copy link
Contributor

clpetix commented Nov 7, 2025

We can just give the input data file and the LAMMPS file so that users have the minimum amount needed to generate the data needed to do the tutorial.

We'll just push a completed tutorial with all cells executed so those who want to see the results without following along have that option too.

@mphoward
Copy link
Contributor

mphoward commented Nov 7, 2025

Please also make sure this example shows how to use multiprocessing!

Comment on lines 129 to 133
if __name__ == '__main__':
num_workers = max(4, multiprocessing.cpu_count())
with multiprocessing.Pool(num_workers) as p:
num_particles = p.map(process_snapshot, traj)
print(num_particles)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary for this to be wrapped in the if statement?

Also, suggest to only use 2 processors for the num_workers. It's the minimum needed to have multiprocessing behavior.

Last, can you just import multiprocessing in this example, rather than putting it in the default imports? It may be obvious enough, but someone will need to figure out they need to import this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The if statetment is necessary since it generates an error otherwise and in the multiprocessing documentation it is implemented the same way.

conftest.py Outdated
Comment on lines 35 to 36
namespace["multiprocessing"] = multiprocessing
namespace["Pool"] = multiprocessing.Pool
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment above, ideally all these changes would be reverted in this file.

@mphoward mphoward merged commit 6c0d315 into main Nov 20, 2025
15 checks passed
@mphoward mphoward deleted the joss-review-mhlbapat-numba branch November 20, 2025 14:22
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