FsGrid & fieldsolver refactoring

[cscjlan]

FsGrid

Vlasiator has been updated to use the refactored FsGrid. This is visible everywhere FsGrid was used in. Some notable differences are dicussed below.

First, there's only a single FsGrid object and it's called fsgrid. It no longer stores any data. The data that used to be stored by the different grids are now owned by a structure called FsData, one for each ex-grid. The data is not passed around by passing the FsData structures, but rather by passing an std::span. You can get an std::span from FsData with the view() method: auto technicalSpan = technicalData.view();

updateGhostCells now takes in an std::span: fsgrid.updateGhostCells(technical); instead of technicalGrid.updateGhostCells().

Data is no longer accessed through the grid.get(i, j, k) method, but rather by contructing an FsStencil, and using it to compute a 1D index from a 3D index:

auto stencil = fsgrid.makeStencil(i, j, k);
// Get the center element ('ooo'), i.e. the one corresponding to i, j, k
const auto center = technical[stencil.ooo()];

// The element in the -x direction from the center, i.e. i - 1, j, k
const auto x_left = technical[stencil.moo()];

This has the benefit that some values that are constant throughout the simulation can be computed once and stored in the fsgrid object. A single FsStencil is constructed for each iteration of a loop (i.e. one for each i, j, k). The constant values can be accessed by the FsStencil when it computes the 1D value from the 3D index it was constructed with.

Fieldsolver

Fieldsolver has been refactored to use the updated FsGrid. Some files have undergone heavier refactoring (ldz_electric_field.cpp), some less heavy. The underlying reason for all of the refactoring is to make it easier to change the fieldsolver to use the updated FsGrid: with less code repetition there's less work to be done (and thus fewer bugs) when changing the code to use the stencil and the spans. An additional benefit was a speed up especially in the electric field computation.

derivatives.cpp and ldz_volume.cpp use the newly added fsgrid.parallel_for, which is an architecture/processor independent way of doing the desired computation. It takes in a lambda, which it calls for each local cell on the fsgrid.

[ykempf]

All right I seem to have botched something(s). However nominally the last 3 commits should have dealt with

• getting rid of the numerous variants of parallel_for_* loop interfaces
• removing RNG inits for perturbed B
• removing capturing gridSpacing by using the newly passed coordinates.

[ykempf]

Oh yeah forgot to push out the fsgrid commit. 🫠

[ykempf]

The rabbit hole of #1161 convinced me that the testpackage diffs are very unlikely to have arisen from this PR. I will thus proceed with the remaining open things here.

[ursg]

Incomplete review so far, I'll go through some more later. But functional approval, of course.
I'm not fully happy with the style in a number of places, but those are things that can... settle over time.

[ursg]

This parallel for has inconsistent indentation with the others above.

[ursg]

I'm completely ok with most of your autoreformattings in this PR, but breaking all these lines in the datareducer file actually makes the whole source file harder to read. Is this something you can semi-easily revert by hand, or should I do that?

[ykempf]

These were all done automatically by Juhana's software, according to the repo's style file. In things I did manually, I tried to stick to some sort of "what was done so far in the team", but I did not revert Juhana's automatic formatting. I can provide the style file I came up with when trying to improve it. I would really strongly suggest from this side of the bay that pliiis, deploy some CI tool doing all formatting for everyone, or at least a style file that works for all. However I stumbled upon options that changed in recentish versions of clang formatting, and/or git-clang-format was not available on major platforms we use (LUMI), so I think the more robust way would be a CI pre-commit or similar feature holding the ground truth and making everything consistent. At the price of having to create that ground truth file...

[cscjlan]

As @ykempf mentioned in a comment down the line, many of these could be "reverted" quite easily by increasing the allowed line width.

[ursg]

"They're more like guidelines anyway".

I'm all in favour of allowing for longer lines here, because the readability actually benefits from the better formatting in many cases.

[ykempf]

So! Set up some way or other for this to be rolled out automatically, fix the formatting options to your desire, and off we go! Some options allow or disallow e.g. one parameter per line when breaking, but also some options were recent enough not to be available on all platforms, so I suppose having one central reference instance will be easiest to maintain (I tried installing git-clang-format on some machine without having to build the whole of LLVM and failed with that...).

[ursg]

likewise, breaking these unit lines just makes things unneccesarily hard to read.

[ursg]

Also this is unnecessary, IMHO

[ursg]

Here's an exception, that I actually appreciate! :)

[ursg]

Same here.

[ursg]

Here's another particularly unpretty formatting example.

[ursg]

And here, only ~50% of the lines have been broken now.

[ursg]

(This is anyway code that probably nobody will ever read through with any intent or detail... so why isn't this just autogenerated anyway?)

[ykempf]

This is inherited directly from Arto's hand-crafted first implementation out of the paper, I/we only changed the arrays, namespaces etc in hiostory but the indices are still as typed by Him in the olden days.

[ursg]

Why aren't all of these spans just collected together into one proper "fsgrids" object by now? I mean, it's great that they live in their own namespace, but for all intents and purposes, they might as well be handed back and forth in a single pointer, if they were sharing a common struct.

[ursg]

(Also, the fieldsolver functions could then be methods of that object)

[ursg]

And now I saw further down that FsGridSolverData does exist. So how about passing that one here?

[ykempf]

I think so far the idea was to pass the needed spans down the chain, which can transparently point to device or host memory in the end.

[cscjlan]

IIRC I kept the function signatures on the vlasiator side as close to the original as possible if it wasn't necessary to change them. But definitely one could just pass FieldSolverData here.

---

I would keep the functionality separate from the FieldSolverData object, i.e. not implement any methods for it. First of all, the FieldSolverData doesn't even own the data, it's just a wrapper over multiple std::spans -- i.e. pointers & lengths -- the original purpose of which was to make the tens of lambda expressions simpler in datareducer.cpp to simplifiy changing the underlying objects, because they can be changed in one place.

Secondly keeping data and functionality separated was one of the major points of the refactoring on the FsGrid side. This way e.g. the initialization of FsData and FieldSolverGrid can happen separately from each other and there are no partially initialized objects: "on this line the MPI initialization has happened for object A, but the data is garbage, but after these lines also the data has been initialized for object A". With separation, FieldSolverGrid is a fully functioning object after construction and likewise for each FsData (albeit they're full of zeroes).

This separation also simplifies e.g. testing of FsData: when it's implemented to work on the GPU, testing it can be done without any references to MPI/OpenMP, since it neither knows nor cares about those.

[ursg]

I see. Ok then, given what we have learned about memory handling in hybrid architectures, it makes a lot of sense to handle the spans individually and carefully.

[ursg]

Hooray, my least favourite source file got even messier now. :)

[ykempf]

Show it some love, come on! I assume the hyperresistivity one will not be much different. :)

[ykempf]

By my eye, most if not all of the lines you disliked in terms of formatting are due to the max character width setting in the config file.