Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 23 commits into from
Mar 12, 2024

Conversation

yuyttenhove
Copy link
Contributor

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
Copy link
Contributor Author

As a simple comparison of the 2 backends:

"sph" "nearest_neighbors"
densities_sph densities_nearest_neighbors

@JBorrow
Copy link
Member

JBorrow commented Feb 26, 2024

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

Copy link
Member

@JBorrow JBorrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments, but looks good.

swiftsimio/visualisation/slice.py Outdated Show resolved Hide resolved
@yuyttenhove
Copy link
Contributor Author

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

@yuyttenhove
Copy link
Contributor Author

That should be it then, I think?

@JBorrow
Copy link
Member

JBorrow commented Mar 1, 2024

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

@yuyttenhove
Copy link
Contributor Author

yuyttenhove commented Mar 1, 2024

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
Copy link
Contributor Author

yuyttenhove commented Mar 5, 2024

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
Copy link
Member

JBorrow commented Mar 6, 2024

Code needs to be formatted.

@JBorrow
Copy link
Member

JBorrow commented Mar 6, 2024

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
Copy link
Contributor Author

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
Copy link
Member

JBorrow commented Mar 12, 2024

Thank you so much for this contribution!

@JBorrow JBorrow merged commit fda13e2 into SWIFTSIM:master Mar 12, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants