Add particle bounding box in the snapshots' cell meta-data

requested review from @jch

See https://github.com/SWIFTSIM/swiftsimio/issues/121 on the swiftsimio side.

Thanks Matthieu! That's exactly what I had in mind.

How are the coordinates box-wrapped (if at all?) at this point in the code? To decide which cells to read I think we probably want the coordinates wrapped to the periodic copy closest to the cell centre so that the min and max are a good measure of the extent of the cell.

Ah good point about box wrapping; I did not think about this.

Where this is calculated, we don't wrap. I guess we don't want to wrap using the whole box as this would lead to weird min/max coordinates. The particles are themselves wrapped back into the box at the writing point. So there is a mismatch here.

If I have a particle going beyond the x-range (say) of the box then that will contribute to its cell's max x. But then it will be wrapped and actually be at a an x coordinate that is smaller than this cell's min x. However, using that min x would be a bad idea as it would cover the whole box. So my feeling is that I should not box-wrap the coordinates here?

Yes, we certainly don't want to wrap into the box here. If the coordinates haven't been wrapped back into the box since the cells were set up (does the code ever do that?) then there's no need to do anything. The particle coordinates will already be the periodic copies closest to their cells.

I suppose we could explicitly wrap into the range cell_centre-boxsize/2 to cell_centre+boxsize/2 to make it clear what we're writing out and guard against future changes to the rest of the code. Not sure if that's worthwhile?

The particles are box wrapped every time we rebuild. At that point all the particles are guaranteed to be inside their cell as well.

Should the logic you mention be part of the the swiftsimio reading routines?

Actually, thinking about it, the min/max as done now are guaranteed to be within [cell_centre - box/2, cell_centre + box/2]. In fact they are guaranteed to not have travelled by more than half a cell size out at most. And since we always have a t least three cells, then it follows that the particles are in the range you mention.

The only problem here is that the particles in the snapshot are box wrapped.

Yes, the coordinates written to the Coordinates dataset and those used to find the bounding boxes will be wrapped differently, but we have to do it that way for the bounding boxes to be useful. We'll just have to be very careful when computing which cells to read in swiftsimio.

Great, then I will proceed and implement the same mechanism for the other particle types.

@jborrow or @bvandenbroucke anything else you'd think is useful to add at this stage?

Do we know which cells are neighbours of a cell and is there a lightweight way of outputting that information? While it is quite straightforward to recover that information from the cell width and centres, this might incur a considerable overhead when reading a snapshot in swiftsimio, especially if the cells are not output in a particular order (are they?).

Would sorting the cells in a specific way help? Or would you prefer some form of information in each cell telling where to find the neighbouring cells in the array?

Sorting them in traditional Morton order would do it. Then we don't need to store additional information.

Morton or just regular C ordering?

Isn't that the same? x>y>z. Anything is fine, as long as it is clear what is done.

Isn't that Morton: https://en.wikipedia.org/wiki/Z-order_curve ?

Okay, C order then. I didn't know Morton turned it around.

added 2 commits

68991a14 - Use a macro to remove duplication of the common code counting non-inhibited particles
f34e8a2d - Apply the same changes (unifying macro) to the gpart subtypes

Compare with previous version

added 1 commit

262b3ddd - Free the memory allocated for the min/max pos in the cell meta-data

Compare with previous version

Add particle bounding box in the snapshots' cell meta-data

Merge request reports

Activity