ammarhakim / gkyl

This is the main source repo for the Gkeyll 2.0 code. Please see gkeyll.rtfd.io for details.
https://gkeyll.readthedocs.io/en/latest/
62 stars 18 forks source link

Simplifications in Vlasov and Gyrokinetic apps + wrapping offset methods from g0 #98

Closed manauref closed 2 years ago

manauref commented 2 years ago

Performed some serious simplifications in the Vlasov and GK apps, primarily to

  1. Get rid of support for arbitrary collision combinations. Now we only support either self-collisions only, or self-collisions with cross collisions where all species collide with each other.
  2. Remove if-statements that existed in the time loop (i.e. in calcCouplingMoments and advance).

Also wrap the array/CartField offset methods from g0. Note that 1. above will have to be re-examined when we re-implement the neutral model, because in that case not all species collide with each other. But the main point is that if species A collides with species B, we must have species B collide with species A, and that will still be true in neutral models.

Checked many regression tests on both CPU and GPU and they all look good.

JunoRavin commented 2 years ago

This looks great Mana! I think this can be merged without issue. Two things I want to make a note of here for our own future records. These are only notes for record-keeping that your commit made me think about. No action is required.

  1. We should move the allocation of the current out of the Maxwell Field. I know why we did this before but at this point I think it's strange that we do this. The current is a species object and should be attached to the species. In this regard we will no longer require logic for checking the individual vdims of each species. We can just call the accumulate_range object on the charge/espilon_0 weighted M1i moment and that will be that (not that we support Vlasov simulations where species have different numbers of velocity dimensions, so also we shouldn't have this logic at all though I recognize why we did it this way)
  2. We should consider eliminating the logic of "checking" whether a moment has already been computed. In general we should compute things as they are needed for a given updater unless we notice a significant performance degradation. Moments are a fraction of the cost compared the LBO or collisionless updates and in production simulations the difference between re-computing and trying to "fetch the already computed moment" will be at most 10 percent (same is true for memory; the extra memory is allocated is a fraction of a percent of a distribution function). Additionally, from my perspective supporting this logic only gets hairier as we prepare for the work to have implicit collisions (and we may strongly consider defaulting to implicit collisions as Dingyun's work progresses, in which case we will have to separately compute the moments for collisions coupling vs. the moments for the collisionless coupling).
manauref commented 2 years ago

@JunoRavin

  1. Yes, this makes sense. I didn't see exactly where to do this in the current infra, but could be done easily.
  2. Indeed, I removed nearly all momentFlags already. I think only the ones for computing cross collision frequencies remain, but I think those will go away when we parallelize over species.