Interpolation Using Particles #141
Conversation
…de the GPU loops of the particle interpolators
|
This PR modifies the following files which are ignored by .lint-ignore: Please consider removing the corresponding patterns from .lint-ignore so that these files can be linted. |
Cpp-Linter Report
|
…t mem where needed)
|
This PR modifies the following files which are ignored by .lint-ignore: Please consider removing the corresponding patterns from .lint-ignore so that these files can be linted. |
|
Thanks for opening this PR, @tamaraevst. It looks like a good start. 🙂 When I do a proper review, I will have more detailed comments on the code. For now, I have some broader points to raise: Persistent particlesSince we usually want to interpolate the same variables at the same points repeatedly during a simulation, we don't want to be adding and redistributing particles each time we want to interpolate. Can you add a method to add a "persistent" interpolation query which can be made once with the particles added just the first time and re-used subsequently? We will need to remember to "redistribute" after a regrid but don't need to if a regrid hasn't happened since the last interpolation call. I believe redistribution is expensive so we should try to minimise it when we can. Maybe the DerivativesThe AMRInterpolator in GRChombo supported interpolating spatial derivatives in each direction of the variables (and this is still present in We might want to defer this to a future PR and just have an error for the moment if derivatives are requested. Derived QuantitiesA key feature we will want is being able to interpolate derived quantities e.g. For now, in this PR perhaps it's worth adding the ability to interpolate data from an arbitrary Unit testCan you make the unit test run on a full "mini" example (i.e. with Development practices
|
- Leave an error message for derivative queries - Track Redistribute() flag and allow the user to override it Other relevant modifications to the interpolation class: - Simplify copying from device to host memory in populate_from_query() - Make check_domain() parity compatible
|
This PR modifies the following files which are ignored by .lint-ignore: Please consider removing the corresponding patterns from .lint-ignore so that these files can be linted. |
Rewrite a unit test for interpolation to include use of GRAMR and some parameter inputs. More details: - Derived vars interpolation handled via Multifabs (for each level) - We do not have parity supported for derived vars (throw errors in these cases for now) - Interpolation test handles a polynomial as a derived variable and checks for errors using analytical expression
|
This PR modifies the following files which are ignored by .lint-ignore: Please consider removing the corresponding patterns from .lint-ignore so that these files can be linted. |
|
This PR modifies the following files which are ignored by .lint-ignore: Please consider removing the corresponding patterns from .lint-ignore so that these files can be linted. |
|
This PR modifies the following files which are ignored by .lint-ignore: Please consider removing the corresponding patterns from .lint-ignore so that these files can be linted. |
|
This PR modifies the following files which are ignored by .lint-ignore: Please consider removing the corresponding patterns from .lint-ignore so that these files can be linted. |
|
This PR modifies the following files which are ignored by .lint-ignore: Please consider removing the corresponding patterns from .lint-ignore so that these files can be linted. |
OK, I think my previous suggestion about persistent particles didn't really make much sense given I have now realised that the way you are currently using the Basically, we need to think of a way of keeping this object throughout the simulation. This way, the particles can be set once before the first interpolation (i.e.
We should add a proper derived quantity this to test but I guess first we need to add the ability to get a vector of
Once parity support has been added for the vector of
Maybe it's easiest to defer this to a future PR? This one is already quite large!
I certainly ran into lots of problems with GPUs when I was working on the puncture tracking in #89. I have yet to go through all of your code in detail but I will keep this in mind when I do. One thing I have just noticed is I think you are implicitly assuming that the query is the same on every rank (or at least only rank 0's query is used). In GRChombo, we had the ability for each process to request different variables and points. This is used e.g. in the apparent horizon finder. Would you be able to add this capability? Note that this will necessarily complicate the communication hence the use of |
There was a problem hiding this comment.
Sorry I only just noticed you modified this file. Would you be able to remove the changes here and open a separate PR if you want to change this?
There was a problem hiding this comment.
Ah I may have ran autoupdate on it, so will revert to the older one on my next commit.
The following logic is enforced in this commit: - Particle interpolator is stored in BHAMR (as well as particles are populated once) - Particle interpolator is also now templated over the number of components - We ask for Redistribute() of particles after regrid - GRAMR contains a very short function to post-process the MultiFabs of derived variables into the correct format for interpolation - GRAMR contains some virtual function to be overriden by BHAMR in order to set up query and populate the particles only once - Handling for parity of derived vars is implemented. Parity handling for both state and derived is to be tested! - Weyl4 is computed in the post-processing step for now; to be addressed later but is needed for WeylExtraction testing
tamaraevst
force-pushed
the
particle-interp
branch
from
91b775a to
28c059e
Compare
This commit reverts to state var extraction in the main example. This example works well. Diagnostic extraction does not work with symmetric BCs for now.
Previous implementation was not allowing to Redistribute() after the regrid
Previous implementation was not allowing to Redistribute() after the regrid
…n/GRTeclyn into particle-interp
|
As we discussed previously in the meeting, I have ran a couple of additional checks on In particular, the user now needs to create an interpolator object for each of the vars/set of vars they want to extract in BHAMR (e.g. there would need to be 2 interpolator objects if one wants to interpolate Psi4 and \chi at the same time). I added some virtual functions to GRAMR to be provided in BHAMR so that the changes in the query can be tracked and population of particles happens only once. Here is an example of \chi extracted at one of the points x=256, y=256, z=255 (or -1 if using symmetry along z). Punctures are plotted over to give an idea of when the two BHs merge. 
Onto Weyl4 next. |
|
I have done/am doing some basic profiling which I will add in a separate comment. This PR is massive so is going to be quite difficult to review. In an effort to make it easier, when you are ready for this to be reviewed, please could you move all of the Again, when you are happy for this to be reviewed, could you also remove the debugging printing? I guess some of it can be kept, cleaned up and conditionally printed depending on some kind of verbosity parameter. Once my AMReX PR (AMReX-Codes/amrex#4780) is merged that will add the ability for |
|
I have done some basic profiling of the code on the CSD3 A100s using AMReX's Tiny Profiler. I made a few modifications to the code on the I used the following parameter file which is similar to Expand parameter fileSingle GPU resultsClick here to expand TinyProfiler output8 GPUs, 2 nodes (4 GPUs per node) resultsClick here to expand Tiny Profiler outputCommentIn the 8 GPU case, we can see that |
Thanks Miren, the |
Yes that's correct but the dependency is just in one direction. None of the rest of the particle interpolator infrastructure depends on the |
These classes and the complete structure of extraction+interpolation can be seen on the newly created branch particle-interp-extraction
These classes and the complete structure of extraction+interpolation can be seen on the newly created branch particle-interp-extraction
|
This PR modifies the following files which are ignored by .lint-ignore: Please consider removing the corresponding patterns from .lint-ignore so that these files can be linted. |
|
This PR modifies the following files which are ignored by .lint-ignore: Please consider removing the corresponding patterns from .lint-ignore so that these files can be linted. |
Address some linter warnings
|
This PR modifies the following files which are ignored by .lint-ignore: Please consider removing the corresponding patterns from .lint-ignore so that these files can be linted. |
Cool, thanks. So I have removed all of the All of the |
|
I have now updated my comment above with the missing profiling info. Sorry for the delay on this. |
mirenradia
left a comment
mirenradia
left a comment
There was a problem hiding this comment.
OK, I have finally gone through all of the changes in this PR. It took quite a long time!
Sorry for the very large number of comments. Many of them are small or straightforward to fix (e.g. just changing variable names).
I think the largest changes which will involve the most refactoring are unsurprisingly in the ParticleInterpolators files so I would probably look at those files first and focus on the most involved changes first.
With my suggested change for this class to compute derived quantities, I'm wondering how much sense the InterpolatorQueryParticle class structure makes. I guess the user still needs to provide the coordinates and the output pointers but specifying the component indices is a bit weird. I haven't thought of a better idea though...
On a couple of files, there are a few changes to e.g. whitespace that we should remove from this PR as they're irrelevant (I've commented on the individual files). Note that if you're using pre-commit (which will force a whitespace check), in order to commit restoring these files to how they were, you will need to pass --no-verify to skip the pre-commit checks.
|
|
||
| // we do not need std::unique_ptr here strictly speaking. We can actually omit | ||
| // this step. | ||
| void GRAMR::convert_derived_multifabs( | ||
| const amrex::Vector<std::unique_ptr<amrex::MultiFab>> &inputs, | ||
| amrex::Vector<const amrex::MultiFab *> &fields) | ||
| { | ||
| fields.clear(); | ||
| fields.reserve(inputs.size()); | ||
| for (auto const &level_content : inputs) | ||
| { | ||
| fields.push_back(level_content.get()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we remove this function? We can instead use amrex::GetVecOfConstPtrs() or amrex::GetVecOfPtrs() defined in AMReX_Vector.H.
There was a problem hiding this comment.
Do we need another parameter file? Can we just remove it?
Hello,
This is my preliminary attempt to replace GRChombo's
AMRInterpolatorwith amrex particles, subsequently called asParticleInterpolators(although we can rename it to whatever you like later on). The relevant folder that will eventually replace theAMRInterpolatoris located inSource/ParticleInterpolators. I borrowedInterpolationQuery.hppfrom the originalAMRInterpolator, although renamed it toInterpolationQueryParticle.hppto not have conflicts, as theAMRInterpolatorstill remains in the repo.The key features:
Tests/using a polynomial is implemented to test the Lagrange interpolation.TO-DO for me on a fresh head:
Modify
check_domain()function that checks whether we are inside domain to be parity aware; otherwise it throws errors when we ask to interpolate inside the domain but on the side where symmetries are applied.So far the code compiles on GPUs, but I need to run an example (I am having some technical difficulties). On CPUs everything is fine.
Please let me know your thoughts.