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
Reorder particle advection for use with mesh deformation #3760
Conversation
baf44e2
to
0c8de83
Compare
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 think this looks very good! The only major concern I have is for the particle properties. Most properties are fine (everything that does not actually store history, but only extracts information at the current state does not care), but the ...strain...
properties and the pT path
property will probably be wrong in the iterated schemes. Could you try storing a copy of the ParticleHandler and resetting that?
@@ -101,13 +101,22 @@ namespace aspect | |||
return *property_manager; | |||
} | |||
|
|||
|
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.
Add another line (3 empty lines is our default between functions in .cc files, 1 line in .h files).
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.
So what to do when there is a mix of 1, 2, and 3 empty lines in the .cc file (as in this case)? I shouldn't adjust them all in this PR right?
bc0bc3c
to
ae5bce5
Compare
I've made the iterated Advection solver schemes still use the old order of particle advection at the end of the timestep. When we have the functionality to reset the ParticleHandler, we can change those 2 schemes as well. |
7de165b
to
558fdce
Compare
Hi @gassmoeller , I've used your copy_particle_handler function (#3818) to also reorder the particle advection in the iterated Advection schemes, thanks! I tested the schemes with the test we set up with @ricitron and with a similar setup that does not deform the mesh but prescribes a non-zero Stokes velocity. In both cases particle positions and reference positions are correct. A van Keken benchmark also runs with a larger number of particles. Could you have another look? Also, a lot of tests will fail due to the reordering of the particle advection. In how far do we have to check whether the diffs make sense? Are there tests with known solutions to check? |
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.
Just some minor comments, in general this looks good. In terms of benchmarking: I think the best benchmark will be benchmarks/rigid_shear
and the simplified test case rigid_shear_time_dependent. This is the time dependent box benchmark from our paper (Gassmöller, R., Lokavarapu, H., Bangerth, W., & Puckett, E. G. (2019). Evaluating the accuracy of hybrid finite element/particle-in-cell methods for modelling incompressible Stokes flow. Geophysical Journal International, 219(3), 1915-1938.)
From the test results it looks like the output changes only slightly (because we now use the extrapolated velocity instead of the real velocity?), but it could be good to confirm by running the full benchmark again (cheap) and not only the test, and we should also run the same benchmark with a nonlinear solver scheme. I am working on getting another benchmark into a PR, but the rigid shear one should be sufficient already.
@gassmoeller. I've now run part of the rigid shear time dependent benchmark (lowest and highest resolution, only for cell average interpolator and for 10, 45 or 80 particles per dim). The change in errors is small and decreases with resolution and with the number of particles: I'll run a different suite with a nonlinear solver scheme. |
Hi @gassmoeller . Here results for the benchmark using the |
The results looks great, thanks! No need for further tests from my point of view, if something would be wrong it should show up prominently in these plots. Can you update the test output and add a changelog entry? Afterwards this should be good to go. |
Sorry to bring up one more point: Could you check what is happening in the viscoelastic tests (e.g. visco_plastic_vep_stress_build-up_yield_particles)? It seems like the properties are now updated one timestep earlier than before, i.e. compositions were 0/0/0 before, and now 1e6/1e6/.... The values are the same as in later timesteps, it just looks like the update happens one timestep earlier than before. Maybe that is even correct, but I would like to understand why it happens. |
Hi @gassmoeller good point! I haven't quite figured it out yet. This is just to help myself remember:
|
This is what happens for the failing vep tests:
First I thought old_stress should be the stress from the last timestep, but without particles, the old_stress would also be the compositional value of the current timestep. So I think it's correct, but I'd like to discuss this with @gassmoeller and @naliboff . I noticed the bending beam benchmark has a field and a particles prm, I'll compare the two. Previously, the particles were advected at the end of the timestep. So at t0, the update of the property elastic stress does nothing, as reaction terms are not evaluated. At t1, this zero stress property is interpolated onto the compositional fields, which are therefore also zero. At the end of timestep t1, the particles are advected and updated and carry nonzero stresses, while the stress compositional fields are still zero (see figure below showing t1=1000yr from the test |
There are also a lot of tests where the particle velocity outputted in the gnuplot file for t0 is now 0. This is because the particles are now advected before there is an actual velocity solution, instead of at the end of the timestep. |
Thanks for checking, all of this looks correct and like an improvement over the current state. If you update the remaining tests I am fine with merging this.
Only if the material model also checks |
e40f62f
to
dc1bb77
Compare
dc1bb77
to
39f0ee4
Compare
Hi @gassmoeller I've stored the particle_handler copy in world.cc, as requested in #3932. If this is what you were looking for, I'll update the test results.
The functions in elasticity.cc do check whether the reaction_terms are requested. |
@anne-glerum, @gassmoeller - the updated elasticity results look correct. In fact, this is an improvement. Due to the order of how the compositional field values for elasticity were updated when tracking elasticity on particles the output compositional field values always appeared to be a one time step off. These listed compositional field values between tests using fields or particles to track the elastic stresses are now in line. Thanks for your work on this and happy to discuss or test in more detail if needed. |
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.
Yes, this is exactly what I had in mind. Go ahead and update the tests.
The functions in elasticity.cc do check whether the reaction_terms are requested.
Oh, yes, then not requesting reaction terms should stop the computation. Feel free to adjust.
{ | ||
copy_particle_handler (particle_handler_backup, *particle_handler.get()); | ||
} | ||
|
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.
one empty line too much :-)
@gassmoeller While updating the tests, I realized that the tests I added in this PR don't have the right results yet. Unfortunately the |
Alright, fixed the test. However, |
Slots (the functions connected to the signals) are executed in the order they were connected. Unfortunately the test connects through the signal connector function (which is called from core.cc:149), while the particle world only connects later in core.cc:418. I see two ways to fix this, both not quite clean:
|
7fec3ef
to
e958a66
Compare
Option 2 worked like a charm, thanks @gassmoeller! Waiting for the updated test results and then this should be done :) BTW, I had to patch the indentation with the tester results, as my local |
@gassmoeller Tests are updated, should be ready. |
Looks like one of the tests is still failing. |
This test now passes on my machine. Are the differences with the tester ok to just update the results? |
Yes, if the test works for you then just update the results from the tester. |
fa11ac2
to
5cc88f8
Compare
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.
Very nice, thanks for the changes!
This commit
DONE: check particle advection is correct for the iterative solver schemes.
DONE: add tests
DONE: change test that checks that particles + mesh deformation fails
TODO: update test results
For all pull requests:
For new features/models or changes of existing features: