SWIFTSIM / swiftsimio

Python library for reading SWIFT data. Uses unyt and h5py.
GNU Lesser General Public License v3.0
18 stars 13 forks source link

New slicing backend (nearest neighbors interpolation) for use with moving mesh #182

Closed yuyttenhove closed 8 months ago

yuyttenhove commented 9 months ago

I implemented a new slicing backend (similar to how there are multiple backend for the projection) which uses nearest neighbor interpolation of the quantities as this is more suitable for moving mesh hydrodynamics.

All tests pass and I modified the relevant ones to also test the new backend. All features of the original slicing method (now still the default and dubbed "sph") should be supported by the new "nearest_neighbors" backend. No default behaviour has changed, so this should be fully backwards compatible.

I also added a few sentences to the documentation.

Do you think this is suitable for including in swiftsimIO? Do you have any remarks/wishes?

yuyttenhove commented 9 months ago

As a simple comparison of the 2 backends:

"sph" "nearest_neighbors"
densities_sph densities_nearest_neighbors
JBorrow commented 9 months ago

Amazing! Love it. Thank you for this contribution; we should definitely include this. Thank you also for so carefully working within the existing framework.

yuyttenhove commented 9 months ago

Thanks for the comments! I resolved most of them and left some questions.

yuyttenhove commented 8 months ago

That should be it then, I think?

JBorrow commented 8 months ago

Looks good. We will need to release a new version (v8.0.0) as this contains api breaking changes.

yuyttenhove commented 8 months ago

I just pushed another commit in which I adjusted the spelling of "neighbors" to "neighbours" (like it was already used in generate_smoothing_lengths)

yuyttenhove commented 8 months ago

It turns out np.cbrt is actually not supported by unyt_arrays (and hence cosmo_arrays). I opened a pull request to add support for it in unyt and changed the get_hsml implementation in the meantime.

JBorrow commented 8 months ago

Code needs to be formatted.

JBorrow commented 8 months ago

Not required for this PR, but you could imagine actually not even needing a tree for this. For densely populated areas, you can just find the particle nearest to the pixel center by looping through all the particles, and for the sparsely populated areas a flood fill after that first procedure would work...

yuyttenhove commented 8 months ago

Not required for this PR, but you could imagine actually not even needing a tree for this. For densely populated areas, you can just find the particle nearest to the pixel center by looping through all the particles, and for the sparsely populated areas a flood fill after that first procedure would work...

Yeah, I did consider that actually, as it will probably be faster, but ended up using the tree method, since I already used it outside swiftwimio and hence was very easy to implement. Something to keep in mind for later probably though.

JBorrow commented 8 months ago

Thank you so much for this contribution!