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

Move ownership and responiblity of particle world to simulator #3294

Conversation

@MFraters
Copy link
Contributor

MFraters commented Nov 19, 2019

Currently the particle world is owned by the particle post-processor. This may have made sense in the beginning, but since particles can be active now it seems to make more sense to me for them to be part of the simulator. Furthermore, I am developing a plugin which needs access to up to date particle information. Currently this is only the case if the user provides that post processor after the particle post processor because the particle post-processor also moves and updates the particles.

Although further reorganization might we wanted in the future, this pull request is limited to just letting the simulator own the particle world and update the particles, so that the order given by the user of the post-processors do not matter any more.

I am still looking at a bug (there seems to be a subscriptor which is not deleted at the end of the simulation) and some of the code can probably be improved, but I thought it might be good to make this pull request now so that changes can be discussed early on.

And thanks @gassmoeller for the help and advice already giving :)

For all pull requests:

For new features/models or changes of existing features:

  • I have tested my new feature locally to ensure it is correct.
@MFraters MFraters changed the title [WIP] move ownership and responiblity of particle world to simulator Move ownership and responiblity of particle world to simulator Nov 20, 2019
@MFraters

This comment has been minimized.

Copy link
Contributor Author

MFraters commented Nov 20, 2019

The tests work again and the code is ready for review.

Copy link
Contributor

gassmoeller left a comment

Very nice, especially keeping all the functionality exactly the same! Just a minor suggestion for rewording, other than that this is ready.

@@ -396,6 +397,16 @@ namespace aspect
postprocess_manager.initialize_simulator (*this);
postprocess_manager.parse_parameters (prm);

if (postprocess_manager.template has_matching_postprocessor<Postprocess::Particles<dim> >())

This comment has been minimized.

Copy link
@gassmoeller

gassmoeller Nov 20, 2019

Contributor

Very nice use of this function!

SimulatorAccess<dim>::get_particle_world() const
{
Assert (simulator->particle_world.get() != nullptr,
ExcMessage("You can not call this function if no such model is actually available."));

This comment has been minimized.

Copy link
@gassmoeller

gassmoeller Nov 20, 2019

Contributor
Suggested change
ExcMessage("You can not call this function if no such model is actually available."));
ExcMessage("You can not call this function if there is no particle world."));
SimulatorAccess<dim>::get_particle_world()
{
Assert (simulator->particle_world.get() != nullptr,
ExcMessage("You can not call this function if no such model is actually available."));

This comment has been minimized.

Copy link
@gassmoeller

gassmoeller Nov 20, 2019

Contributor
Suggested change
ExcMessage("You can not call this function if no such model is actually available."));
ExcMessage("You can not call this function if there is no particle world."));
@gassmoeller

This comment has been minimized.

Copy link
Contributor

gassmoeller commented Nov 20, 2019

Oh and could you squash your commits? There are some that would not be useful to have in master.

@MFraters MFraters force-pushed the MFraters:move_particle_advection_out_of_postprocess branch from c1d9695 to 3f74e99 Nov 20, 2019
@MFraters

This comment has been minimized.

Copy link
Contributor Author

MFraters commented Nov 20, 2019

Thanks for the quick review! I have addressed all your comments.

@tjhei
tjhei approved these changes Nov 20, 2019
@gassmoeller gassmoeller merged commit 8186dd8 into geodynamics:master Nov 20, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins.tjhei.info/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.