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

Step-68 : Particle tracking of a massless tracer in a vortical flow #10308

Merged
merged 36 commits into from Sep 18, 2020

Conversation

blaisb
Copy link
Member

@blaisb blaisb commented May 22, 2020

Preliminary version of a step on particle tracking in an analytically defined velocity field.
It is very simple, but it showcases particles in parallel. It uses an analytically defined velocity field to move the particles either by interpolating from an MPI::Vector (e.g. from Trilinos) or analytically from a function. It uses the DiscreteTime class for time-stepping, a simple Euler scheme and ParameterAccessor for parsing (similar to step-70). The only thing missing that would be very interesting would be to add load balancing using something like 1000+1000*nb_particles_per_cell as a weight. I tried looking over the CellWeight class but I think I will need some help figuring out how to use it outside of it's original intended scope. I think Rene already did something similar for ASPECT. Furthermore, once we figure out a generic data out for particle properties, we could attach the mpi rank to the particles.

This is a work in progress and there is no documentation yet.

@peterrum @gassmoeller

@peterrum peterrum added this to the Hackathon 2020 milestone May 22, 2020
Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

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

Looks good!

Are you planing to also add also an advection solver? I would be fine, if you wouldn't, and you would concentrate on particle-related aspect.

Comment on lines 343 to 346
const auto pic = particle_handler.particles_in_cell(cell);

for (unsigned int i = 0; particle != pic.end(); ++particle, ++i)
{
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't you replace this by an iterator?


particle_velocity[comp_j.first] +=
fluid_fe.shape_value(j, reference_location) *
field_relevant(dof_indices[j]);
Copy link
Member

Choose a reason for hiding this comment

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

I would read the values first from the global vector into a cell-local vector and only operate with j here.

@peterrum
Copy link
Member

@gassmoeller

Two questions regarding the usability of the ParticleHandler. Wouldn't it be possible to teach the get_surrounding_cell() function to take also a DoFHandler and return in this case the right type of iterator? This would simplify the following code

        const auto &cell =
          particle->get_surrounding_cell(background_triangulation);
        const auto &dh_cell =
          typename DoFHandler<dim>::cell_iterator(*cell, &fluid_dh);
        dh_cell->get_dof_indices(dof_indices);

to:

        const auto cell = particle->get_surrounding_cell(fluid_dh);

The following code loop over all cells that contain cells and within that cell it loops over all particles:

    auto particle = particle_handler.begin();
    while (particle != particle_handler.end())
      {
        const auto &cell = particle->get_surrounding_cell(triangulation);
        const auto pic = particle_handler.particles_in_cell(cell);

        for (unsigned int i = 0; particle != pic.end(); ++particle, ++i) 
          /*do something*/
      }

Wouldn't be possible to introduce appropriate iterators? step-70 already contained these lines of code?

If you want, I could help and open PRs regarding to these points.

@blaisb
Copy link
Member Author

blaisb commented May 22, 2020

Looks good!

Are you planing to also add also an advection solver? I would be fine, if you wouldn't, and you would concentrate on particle-related aspect.

I think I prefer to concentrate on the particle aspects. I think there are already many tutorials that describe solving equations in parallel. I just envision this step has a very simple, barebone step that focuses on everything related to basic operations on particles.

@gassmoeller
Copy link
Member

I am looking into adding load balancing right now. Once I have something working lets have a chat so I can walk you through the steps. Also I will look into your questions Peter.

@gassmoeller
Copy link
Member

Load balancing is in lethe-cfd#1

@peterrum:
Yes adding another function get_surrounding_cell for DoFHandler iterators would be useful and easy to implement. I will try to get to it within the next days. I opened #10314 to remind us of the issue.

    typename Triangulation<dim, spacedim>::cell_iterator
    get_surrounding_cell(
      const Triangulation<dim, spacedim> &triangulation) const;

    typename DoFHandler<dim, spacedim>::cell_iterator
    get_surrounding_cell(
      const DoFHandler<dim, spacedim> &dof_handler) const;

The second case is a bit more complicated. I think Bruno changed it already for this step, which is possible here. But the general issue (if I remember correctly) is that the following three ways to do the same thing (iterating over all particles) have very different performance characteristics:

for (particle: particles)
{
cell = particle->get_surrounding_cell()
... do something
}

for (cell: cells)
{
particles = particle_handler.find_particles_in_cell(cell)

for (particle: particles)
{
... do something
}
}

for (particle: particles)
{
cell = particle->get_surrounding_cell();
particles_in_cell = particle_handler.find_particles_in_cell(cell)

for (particle_in_cell: particles_in_cell)
{
... do something and advance particle_in_cell **and** particle
}
}

I think there were cases where the third version is necessary. Were you suggesting to add new particle iterators that would only iterate over the particles of a single cell? That could cover the third case. I would be interested in hearing how you would do that, my knowledge about iterators is still a bit limited.

@blaisb blaisb changed the title Step on particle tracking Step-68 : Particle tracking of a massless tracer in a vortical flow May 25, 2020
@blaisb
Copy link
Member Author

blaisb commented May 25, 2020

@peterrum I reserved step-68 like you suggested by putting it in the PR's name. Is that ok?
I should finish first run of documentation and results tomorrow since Rene added load balancing.
:)!

@blaisb
Copy link
Member Author

blaisb commented May 28, 2020

Dear  @gassmoeller and @peterrum. I finished a first full version of the tutorial with documentations. Rene there are two very minor places where I would appreciate your input for the documentation.

The results are on the following youtube links:
https://youtu.be/EbgS5Ch35Xs
https://youtu.be/ubUcsR4ECj4

As always, I am grateful for all your comments and ideas :)

Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

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

Very nice! I have bunch of minor requests.

examples/step-68/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-68/doc/intro.dox Outdated Show resolved Hide resolved
\frac{d \textbf{x}_i}{dt} =\textbf{u}(\textbf{x}_i)
@f]

where $\textbf{x}_i$ is the postion of particle $i$. In the present step,
Copy link
Member

Choose a reason for hiding this comment

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

... and u its verlocity.

examples/step-68/doc/intro.dox Outdated Show resolved Hide resolved
Comment on lines 60 to 61
of the motion of the particles. However, this does not alter which capacities
are displayed in the present step.
Copy link
Member

Choose a reason for hiding this comment

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

does not alter which capacities are displayed

sounds odd...

Comment on lines 8 to 30
Regardless of the specific parameter file name, if the specified file does not
exist, when you execute the program you will get an exception that no such file
can be found:

@code
----------------------------------------------------
Exception on processing:

--------------------------------------------------------
An error occurred in line <74> of file <../source/base/parameter_acceptor.cc> in function
static void dealii::ParameterAcceptor::initialize(const std::string &, const std::string &, const ParameterHandler::OutputStyle, dealii::ParameterHandler &)
The violated condition was:
false
Additional information:
You specified <parameters.prm> as input parameter file, but it does not exist. We created it for you.
--------------------------------------------------------

Aborting!
----------------------------------------------------
@endcode

However, as the error message already states, the code that triggers the
exception will also generate the specified file ("`parameters.prm`" in this case).
Copy link
Member

Choose a reason for hiding this comment

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

Not needed!

examples/step-68/doc/results.dox Outdated Show resolved Hide resolved
examples/step-68/doc/results.dox Outdated Show resolved Hide resolved
examples/step-68/doc/results.dox Outdated Show resolved Hide resolved
this case the particles would need to have additional properties such as their mass,
and their diameter.
- Coupling to a flow solver. This step could be straightforwardly coupled to any parallel
steps in which the Stokes or the Navier-Stokes equations are solved (e.g. step-57)
Copy link
Member

Choose a reason for hiding this comment

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

You could add a reference to step-70.

Copy link
Member

Choose a reason for hiding this comment

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

and step-32

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Looks very nice! Let me know if you want me to take another look after merging my changes and addressing @peterrum and my comments.

examples/step-68/doc/results.dox Outdated Show resolved Hide resolved
examples/step-68/doc/results.dox Outdated Show resolved Hide resolved
this case the particles would need to have additional properties such as their mass,
and their diameter.
- Coupling to a flow solver. This step could be straightforwardly coupled to any parallel
steps in which the Stokes or the Navier-Stokes equations are solved (e.g. step-57)
Copy link
Member

Choose a reason for hiding this comment

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

and step-32

examples/step-68/doc/tooltip Outdated Show resolved Hide resolved
@blaisb
Copy link
Member Author

blaisb commented May 29, 2020

Please do not review this commit. I might (possibly) have identified a bug in insert_global_particles and that hindered my progression with adding the properties to the particles.

@blaisb
Copy link
Member Author

blaisb commented Jun 2, 2020

This branch is still not ready for final review. I am sorry. I am facing what appears to be a bug with the data out of particles in parallel if the particles have properties, I will open an issue

@blaisb
Copy link
Member Author

blaisb commented Jun 2, 2020

Dear @peterrum and @gassmoeller,
I think this is a first finished version of step-68 with all the features I wanted in it. Major changes :

  • Removed all dependency on trilinos and petsc. Now everything is done using deal.II distributed vectors
  • Added properties to the particles which are also postprocessed.

I would greatly welcome your comments and your reviews? I don't plan on adding new features, so I think this is ready for a final round of review. All of your comments are more than welcomed :)

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Very nice! I made some text suggestions, but they are all minor.

The only bigger question I have is that you claim in the results that this tutorial highlights some of the main capabilities of particles, notably their capacity to be used in distributed parallel simulations. I feel that for that to be true we would need to add a few paragraphs in the introduction that highlight what is necessary to use particles in distributed simulations, and a few paragraphs in the results that point out where and when this happens in the example. Specifically, I think we should shortly discuss the following processes:

  • Generating the particles using a distributed triangulation that is different from the triangulation we use for the flow. This allows a convenient parallel creation of particles, but requires a redistribution of particles after the generation, which can be expensive. Nevertheless, since it is a one-time process it is often worth the convenience over algorithms that only generate particles on the local process (which are also available in deal.II, e.g. the Particles::Generators::regular_reference_locations used with the flow triangulation or Particles::Generators::probabilistic_locations with a probability density function
  • The exchange of particles that leave a local domain, including mentioning that this is only efficient (and in fact only implemented at the moment) if particles move to the ghost layer of the local process. Distributing particles to elements outside the ghost layer has to be implemented manually, e.g. like in step-70.
  • The importance of load balancing, and how particles are shipped during mesh repartition. That we use the cell_weight function to influence the normal deal.II (in fact p4est) repartition algorithm, and that particle data is serialized and attached to cells just like solution vector data and deserialized on the receiving process.

Do you think we should a paragraph for each of these points? I can give particle transfer and load balancing a try, do you want to take a shot at particle generation?

and Peter Munch (Technical University of Munich and Helmholtz-Zentrum Geesthacht)
</i>

@dealiiTutorialDOI{10.5281/zenodo.3829064,https://zenodo.org/badge/DOI/10.5281/zenodo.3829064.svg}
Copy link
Member

Choose a reason for hiding this comment

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

The doi needs to be updated or removed when finished.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are absolutely right.
I felt that the parallel load balancing, although very interesting, appeared out of the blue. I really love your three suggestions as they are very didactic and that would improve the quality of the step for users who are not experience in particle tracking. I will give a shot at parallel particle generation and you can give a shot at load balancing and particle transfer like you suggested. I will also integrate your other comments :). Thank you very much for taking the time to review and contribute this much :)!


Particles play an important part in numerical models for a large
number of applications. Particles are routinely used
as massless tracer to visualize the dynamic of a transient flow. They
Copy link
Member

Choose a reason for hiding this comment

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

Maybe?

Suggested change
as massless tracer to visualize the dynamic of a transient flow. They
as massless tracers to visualize the dynamic of a transient flow. They

number of applications. Particles are routinely used
as massless tracer to visualize the dynamic of a transient flow. They
can also play an intrinsic role as part of a more complex finite element
model, as is the case of the Particle-In-Cell (PIC) method (Gassmöller et al. 2018)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
model, as is the case of the Particle-In-Cell (PIC) method (Gassmöller et al. 2018)
model, as is the case for the Particle-In-Cell (PIC) method (Gassmöller et al. 2018)

Comment on lines 23 to 24
or they can even be used to simulate the motion of granular matter, as is
the case with the Discrete Element Method (DEM) (Blais et al. 2019). In the case
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
or they can even be used to simulate the motion of granular matter, as is
the case with the Discrete Element Method (DEM) (Blais et al. 2019). In the case
or they can even be used to simulate the motion of granular matter, as in
the Discrete Element Method (DEM) (Blais et al. 2019). In the case

of DEM, the resulting model is not related to the finite element method anymore,
but just leads to a system of ordinary differential equation which describes
the motion of the particles and the dynamic of their collisions. All of
these models can be built using deal.II particle handling capabilities.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
these models can be built using deal.II particle handling capabilities.
these models can be built using deal.II's particle handling capabilities.


We note that, by default, the simulation runs the particle tracking with
an analytical velocity for 2000 iterations, then runs the particle tracking with
velocity interpolationn for the same duration. The results are written every
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
velocity interpolationn for the same duration. The results are written every
velocity interpolation for the same duration. The results are written every

examples/step-68/doc/results.dox Show resolved Hide resolved

<h3>Possibilities for extensions</h3>

This steps highlights some of the main capabilities of particles, notably their
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This steps highlights some of the main capabilities of particles, notably their
This steps highlights some of the main capabilities for handling particles in deal.II, notably their

this case the particles would need to have additional properties such as their mass,
and their diameter.
- Coupling to a flow solver. This step could be straightforwardly coupled to any parallel
steps in which the Stokes (step-32, step-70) or the Navier-Stokes equations are solved (e.g. step-57)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
steps in which the Stokes (step-32, step-70) or the Navier-Stokes equations are solved (e.g. step-57)
steps in which the Stokes (step-32, step-70) or the Navier-Stokes equations are solved (e.g. step-57).

<h3>Possibilities for extensions</h3>

This steps highlights some of the main capabilities of particles, notably their
capacity to be use in distributed parallel simulations. However, this step could
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
capacity to be use in distributed parallel simulations. However, this step could
capacity to be used in distributed parallel simulations. However, this step could

@blaisb blaisb force-pushed the particle_advection_step branch 2 times, most recently from 76e3037 to 5d8b9a4 Compare June 9, 2020 23:45
@blaisb
Copy link
Member Author

blaisb commented Jun 10, 2020

@gassmoeller
Thank you so much for your comments. I added all of your suggestions and modified the notation. I also added a new section in the introduction called : Challenges related to distributed particle simulations

In this section, I added the discussion on the global generation of the particles. The two elements missing would be load balancing and particle exchange between the processors. I think that adding this to the tutorial makes the step a lot easier to follow and a lot more didactic. I will be waiting for your two paragraph and I think that after this the step should be finished and ready for final review :). I am very happy about the result.

@blaisb
Copy link
Member Author

blaisb commented Jun 24, 2020

ping @gassmoeller

@gassmoeller
Copy link
Member

Sorry for the radio silence. I sent my paragraphs in lethe-cfd#3 a while ago, but I did not mention that here so it got lost. I will read through the tutorials as it is right now, my changes in the other PR are pretty self-contained.

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Looks nearly ready. I have some text suggestions, and I think there are some suggestions from @peterrum that are not yet addressed (like renaming euler_analytical into euler_step_analytical). If you want me to take care of any of that let me know, but I think it is quicker to fix this on your side (without going through another PR from me).

Comment on lines 76 to 78
diameter, temperature, etc.). In the present tutorial, this is used to
store the value of the fluid velocity and the process id to which the particles
belong.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
diameter, temperature, etc.). In the present tutorial, this is used to
store the value of the fluid velocity and the process id to which the particles
belong.
diameter, temperature, etc.). In the present tutorial, this is used to
store the value of the fluid velocity and the process id to which the particles
belong.


<h3>Challenges related to distributed particle simulations</h3>

Although the present step is not computationnaly intensive, simulations that
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Although the present step is not computationnaly intensive, simulations that
Although the present step is not computationally intensive, simulations that

<h3>Challenges related to distributed particle simulations</h3>

Although the present step is not computationnaly intensive, simulations that
include particles can be computationnaly demanding and require parallelization.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
include particles can be computationnaly demanding and require parallelization.
include particles can be computationally demanding and require parallelization.

Generating the particles is not straightforward since the processor to which they belong
must first be identified before the cell in which they are located is found.
Deal.II provides numerous capabilities to generate particles through the Particles::Generator namespace.
Some of these particle generator generate particles on the locally own subdomain. For example,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Some of these particle generator generate particles on the locally own subdomain. For example,
Some of these particle generators generate particles on the locally owned subdomain. For example,

does not constitute a large portion of the computational cost. For these occasions, deal.II provides
convenient Particles::Generators that can globally insert the particles even if they are not located
on the subdomain from which they are created. The generators first locate on which subdomain the particles
are situated, identify is which cell they are located and exchange the necessary information amongst the processors
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
are situated, identify is which cell they are located and exchange the necessary information amongst the processors
are situated, identify in which cell they are located and exchange the necessary information amongst the processors

the dynamics of a particular vortical flow : the Rayleigh-Kotte Vortex. This flow pattern
is generally used as a complex test case for interface tracking methods
(e.g. volume-of-fluid and level set approches) since
it leads to strong rotation and elongation of the fluid (Blais 2013).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it leads to strong rotation and elongation of the fluid (Blais 2013).
it leads to strong rotation and elongation of the fluid (Blais, 2013).

examples/step-68/doc/results.dox Outdated Show resolved Hide resolved
examples/step-68/doc/results.dox Outdated Show resolved Hide resolved
v &=& \frac{\partial\Psi}{\partial x} = 2 \cos(\pi x) \sin(\pi x) \sin^2 (\pi y) \cos \left( \pi \frac{t}{T} \right)
@f}


Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a sentence like the following explaining that the flow will reverse periodically and material will end up where it started:

It can be seen that this velocity reverses periodically due to the term $\cos \left( \pi \frac{t}{T} \right)$ and that material will end up at its starting position after every period of length $t=2T$. We will run this tutorial program for exactly one period and compare the final particle location to the initial location to illustrate this flow property.

examples/step-68/step-68.cc Outdated Show resolved Hide resolved
@blaisb
Copy link
Member Author

blaisb commented Jul 16, 2020

Sorry for the radio silence. I sent my paragraphs in lethe-cfd#3 a while ago, but I did not mention that here so it got lost. I will read through the tutorials as it is right now, my changes in the other PR are pretty self-contained.

My bad, I am sorry. I did not get that pull request warning (or it got lost through the mail).
I will work on integrating your comments right away :)!.

@blaisb
Copy link
Member Author

blaisb commented Jul 16, 2020

@gassmoeller @peterrum
I integrated all of your suggestions. I also rebased the code on the latest deal.II version and I did some additional cleaning (e.g. fixing spelling mistakes and syntax in some places. I think this is ready for the final review :)!

Sorry for the delay in merging your pull request @gassmoeller

Copy link
Member

@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.

Comments about the introduction.

My apologies for the long delay -- the start of the semester has been a bitch :-(

examples/step-68/doc/builds-on Show resolved Hide resolved
doc/doxygen/tutorial/tutorial.h.in Outdated Show resolved Hide resolved
examples/step-68/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-68/doc/intro.dox Show resolved Hide resolved
examples/step-68/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-68/doc/intro.dox Show resolved Hide resolved
examples/step-68/doc/intro.dox Show resolved Hide resolved
examples/step-68/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-68/doc/intro.dox Outdated Show resolved Hide resolved
examples/step-68/doc/intro.dox Outdated Show resolved Hide resolved
Copy link
Member

@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.

And some on the results section.

examples/step-68/doc/results.dox Outdated Show resolved Hide resolved
examples/step-68/doc/results.dox Show resolved Hide resolved
examples/step-68/doc/results.dox Outdated Show resolved Hide resolved
examples/step-68/doc/results.dox Outdated Show resolved Hide resolved
examples/step-68/doc/results.dox Outdated Show resolved Hide resolved
@blaisb
Copy link
Member Author

blaisb commented Sep 16, 2020

I applied all of Wolfgang's comment. Thank you very much for taking the time to review the tutorial and for the suggestions :)! The tutorial now assumes that step-19 exists (it depends on step-19). I think it is ready to be merged, but that's your call. I would merge it after step-19 though :)!
@peterrum @bangerth

That was fun work, once this one is merged I will start work on another example.

@peterrum
Copy link
Member

Do you want to to squash your commits?

Added that the tutorial for the particles build on step-40
which I think is the first complete parallel tutorial.
Added changelog entry in major changes
Fixed additional spelling mistake
Apply suggestions from code review by bangerth
Applied all additional suggestions by Wolfgang Bangerth
- Fixed spelling mistakes
- Added an animation for the Rayleigh-Kothe vortex in the introduction
to illustrate the flow pattern

Co-authored-by: Wolfgang Bangerth <bangerth@colostate.edu>
@blaisb
Copy link
Member Author

blaisb commented Sep 16, 2020

Comments about the introduction.

My apologies for the long delay -- the start of the semester has been a bitch :-(

Don't worry about it. Same story here.

Do you want to to squash your commits?

"Gotta squash em all, gotta squash em all...."
Sorry. I squashed all of the commits from September into one. Hopefully this should do it
:)!

Copy link
Member

@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.

First set of comments on the .cc file. Rest later today. Only small stuff, should be easy to address!

examples/step-68/step-68.cc Outdated Show resolved Hide resolved
examples/step-68/step-68.cc Outdated Show resolved Hide resolved
examples/step-68/step-68.cc Outdated Show resolved Hide resolved
examples/step-68/step-68.cc Outdated Show resolved Hide resolved
examples/step-68/step-68.cc Outdated Show resolved Hide resolved
examples/step-68/step-68.cc Outdated Show resolved Hide resolved
examples/step-68/step-68.cc Outdated Show resolved Hide resolved
examples/step-68/step-68.cc Show resolved Hide resolved
examples/step-68/step-68.cc Outdated Show resolved Hide resolved
examples/step-68/step-68.cc Outdated Show resolved Hide resolved
@drwells
Copy link
Member

drwells commented Sep 16, 2020

I think we are at the point (like in step-69) where, nearing 200 comments, we are likely to start breaking GitHub. @bangerth if there are more large change sets could you collect them into a PR against this branch on lethe-cfd?

@bangerth
Copy link
Member

Yes, good point. I keep forgetting. My remaining edits are all here: lethe-cfd#4

Make whitespace more uniform.

Edit euler_step_interpolated() somewhat.

Leave a note about step-68.

Edit the remainder of the program.

Apply suggestions from code review by bangerth

Co-authored-by: Wolfgang Bangerth <bangerth@colostate.edu>

Applied the final round of comments from Wolfgang
@blaisb
Copy link
Member Author

blaisb commented Sep 17, 2020

@bangerth I applied all of your comments but there are two points that remain unresolved. Could you look at them in the file tab of this PR? :) Thank you so much for the review.

Copy link
Member

@drwells drwells left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines 418 to 421
background_triangulation.signals.pre_distributed_repartition.connect(
std::bind(
&Particles::ParticleHandler<dim>::register_store_callback_function,
&particle_handler));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
background_triangulation.signals.pre_distributed_repartition.connect(
std::bind(
&Particles::ParticleHandler<dim>::register_store_callback_function,
&particle_handler));
background_triangulation.signals.pre_distributed_repartition.connect(
[this]() {this->particle_handler.register_store_callback_function(); });

Copy link
Member Author

Choose a reason for hiding this comment

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

This works for the register_store_callback_function.
However, the register_load_callback_function expects an argument in the form of a serialization (see https://www.dealii.org/current/doxygen/deal.II/classParticles_1_1ParticleHandler.html#a9a79889c1ad3b569a405488d4bab8263)
How do I deal with this? Because
background_triangulation.signals.post_distributed_repartition.connect( [this]() { this->particle_handler.register_load_callback_function(); });
does not work obviously since it is missing an argument. Sorry again for asking, but my experience with signals or lambdas is extremely limited.

Copy link
Member

Choose a reason for hiding this comment

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

Cannot you simply write?

    background_triangulation.signals.post_distributed_repartition.connect(
      [&](){this->particle_handler.register_load_callback_function(false)});

Copy link
Member Author

@blaisb blaisb Sep 18, 2020

Choose a reason for hiding this comment

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

Ok this seems to compile well. But now the code segfaults in parallel when partitioning. I'll try to debug and see what happens. Some change somehow must have broken things.

@peterrum
Copy link
Member

@blaisb There seems to be a merge conflict here. Could you fix it. After that I think it will be merged!

@blaisb
Copy link
Member Author

blaisb commented Sep 17, 2020

@blaisb There seems to be a merge conflict here. Could you fix it. After that I think it will be merged!

Yeah no problem, I will take care of that tomorrow morning first thing. I'll message you after that.

Fixed a bug related to deal.II distributed vector and to the particle
loop in interpolated euler.
@bangerth
Copy link
Member

If you can't figure it out, just leave it as you had it and one of us takes care of it as a follow-up.

@drwells
Copy link
Member

drwells commented Sep 18, 2020

@bangerth does that comment signify your approval with the current state? If so lets merge!

@bangerth
Copy link
Member

Yes! I've got to run, which is typically a bad sign to commit or merge anything, but happy to let someone else do it who can devote a minute or two of thought to the process!

Toni El Geitani Nehme (Polytechnique Montréal),
Rene Gassmöller (University of California Davis),
and Peter Munch (Technical University of Munich and Helmholtz-Zentrum Geesthacht).
Bruno Blais was supported by NSERC Discovery grant
Copy link
Member

Choose a reason for hiding this comment

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

It took me a minute to realize that this is not a typo for NERSC :)

@drwells drwells merged commit e7bb9ce into dealii:master Sep 18, 2020
@blaisb
Copy link
Member Author

blaisb commented Sep 18, 2020

If you can't figure it out, just leave it as you had it and one of us takes care of it as a follow-up.

I figured it out.
It now runs in debug mode in parallel at any number of cores on two of my machines.
There were some minor issues with vectors and the interpolated euler loop.

@blaisb
Copy link
Member Author

blaisb commented Sep 18, 2020

I would like to take some time to thank you all for the reviews. It was a great learning opportunity for me and I am glad I was able to make a durable contribution to a library that has really been a blessing since I started my career as a professor. Thank you for your time. I know we are all swamped by so many tasks and I appreciate every moment of the time you took to help.
@drwells @bangerth @peterrum @gassmoeller

@blaisb blaisb deleted the particle_advection_step branch September 18, 2020 17:32
@bangerth
Copy link
Member

The pleasure is all ours! We hope for more contributions from you and your group! :-)

@peterrum
Copy link
Member

For some reason the videos are not displayed in my case properly? Does anyone have the same issue? step-19 is fine!?

@blaisb
Copy link
Member Author

blaisb commented Sep 20, 2020

For some reason the videos are not displayed in my case properly? Does anyone have the same issue? step-19 is fine!?

Yeah they are not displayed correctly on mine also. I think the issue is because i did not use a /embed/ link like Wolfgang did in step-19. I'll make a PR for that tomorrow. :)!.

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.

None yet

7 participants