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

Rename particle world to particle manager #6058

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

gassmoeller
Copy link
Member

This is a suggestion resulting from the discussion in the user meeting yesterday about a better name for the particle world class. See my reasoning for the renaming here. I think the most convincing suggestion was to try the name Manager, because it most directly explains the purpose of the class, and it is nicely analogous to all the existing manager classes we have already. Part of me is sad to see World go, because the name had something poetic about it (all the particles living in their own world 😄). But I think the new term is better, and the implementation was simpler than expected. I even found a way to make moving the files and renaming the class backward compatible by keeping the main header file around and by declaring an alias for the World class name. This PR is mostly a lot of search and replace, but in essence it does:

  1. Rename all instances of world to manager, incl. the Particle::World class name, the simulator access functions, and the new cmake variable and input/output parameters.
  2. Move the files world.[h,cc] to manager.[h,cc]. Also rename all existing test files.
  3. Add a new world.h header file that enables backward compatibility for existing user plugins.
  4. Add an update script that should update calls to simulator_access.get_particle_world() to the new simulator_access.get_particle_manager(0). I assumed we would mostly update files from before Add further multiple particle world instrastructure. #5920 which introduced the new function parameter.
  5. I moved the input parameter Number of particle managers from the global subsection into the Particles subsection. I think this is more appropriate and aligns closer with how things are done for compositional fields (Compositional fields/Number of compositional fields).

@MFraters, @tjhei, @bangerth: You were most closely involved in the discussion, can you take a look? It is a lot of changes, but almost all of it is simple search/replace.

@tjhei
Copy link
Member

tjhei commented Oct 8, 2024

I am not sure I expressed my opinion on the naming well yesterday (for what it is worth, I don't feel strongly about it). For me:
"particle system" > "particle world" > "particle manager".

The word "manager" doesn't make it obvious to me why you would want more than one of them.

@gassmoeller
Copy link
Member Author

I see your point from the user (input/output) perspective. However, I think System is not a good name for the class, because as Wolfgang mentioned the purpose of the class is the active process of managing the particles (a lot of the class documentation already spoke about managing the particles before my PR).

Do you think it would be confusing if we call the input/output options particle "system" ("Number of particle systems", "Particle system 2", ...), and keep particle "manager" for everything inside the source code ("this->get_particle_manager(x)")? This would make some sense to me as in: If the user requests multiple particle systems in their model, we need multiple particle manager objects to manage those systems.

@tjhei
Copy link
Member

tjhei commented Oct 14, 2024

Sure, fine with me.

@gassmoeller
Copy link
Member Author

Ok, I implemented that and updated the test input and output in the last commit. Is this a version we can all agree on @bangerth @tjhei @MFraters (and anyone else who reads this)? Then I can rebase and also rename the tests (from multiple_managers to multiple_systems).

Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

There is a downside to naming classes differently than run time parameters in that it makes it more difficult to mentally map from one to the other and find where a parameter matches a concrete class. But I guess the downside is relatively minor here, and I can see @tjhei's point about not wanting the name "manager" in the input file -- that makes sense to me too.

So let's go with this!

@bangerth
Copy link
Contributor

OK to merge from my side, but you've got a bunch of merge conflicts.

@tjhei tjhei merged commit ad59386 into geodynamics:main Oct 17, 2024
8 checks passed
@gassmoeller gassmoeller deleted the rename_particle_managers branch October 18, 2024 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants