trixi-framework / TrixiParticles.jl

TrixiParticles.jl: Particle-based multiphysics simulations in Julia
https://trixi-framework.github.io/TrixiParticles.jl/
MIT License
33 stars 10 forks source link

Use default bounding box for `FullGridCellList` #665

Open efaulhaber opened 3 days ago

efaulhaber commented 3 days ago

As suggested in https://github.com/trixi-framework/TrixiParticles.jl/pull/660 by @svchb, it would be more convenient if the minimum and maximum coordinates of the semidiscretization would automatically be used for the bounding box.

Here, we want to create a cell list template without a bounding box, and the semidiscretization should set the bounding box when copying the NHS.

cell_list = TrixiParticles.PointNeighbors.FullGridCellList()
semi = Semidiscretization(fluid_system, boundary_system,
                          neighborhood_search=GridNeighborhoodSearch{2}(; cell_list))

Here, we want to use a larger bounding box, so the semidiscretization should not overwrite an existing bounding box of the cell list.

cell_list = TrixiParticles.PointNeighbors.FullGridCellList(; min_corner, max_corner)
semi = Semidiscretization(fluid_system, boundary_system,
                          neighborhood_search=GridNeighborhoodSearch{2}(; cell_list))

That would make the copy_neighborhood_search API awkward.

copy_neighborhood_search(min_corner=..., max_corner=...)

would only use min_corner and max_corner to set the bounding box when there is none in the template. When the cell list already has a bounding box, these arguments would have to be ignored, or the second snippet above would not work. This sounds like a bad API to me.

svchb commented 3 days ago

Well I would suggest rather modifying this:

semi = Semidiscretization(fluid_system, boundary_system, data_type=CuArray)
function Semidiscretization(systems...; neighborhood_search=NeighborhoodSeach(data_type), data_type=nothing)

So it defaults to FullGridCellList with the default bounding_box when data_type=CuArray is set.

In case a non default bounding box needs to be set

nhs = NeighborhoodSearch(data_type=CuArray, min_corner, max_corner)
semi = Semidiscretization(fluid_system, boundary_system, data_type=CuArray)

or the option to use it like it is now. That would allow switching between GPU/CPU with just modifying data_type in examples since the boundary box can still be provided and then be ignored for neighborhood search that don't require them.

efaulhaber commented 3 days ago

Good idea, but I don't like the fact that a different data type would silently change the underlying NHS implementation, which could be confusing, especially for performance comparisons. @LasNikas what do you think?

Note that this is supposed to be more or less temporary, as we want to have a GPU-compatible NHS with unbounded domain in the future. But it will probably not happen soon.

efaulhaber commented 3 days ago

Also note that FullGridCellList is slightly faster on CPUs as well, and the fully parallel update is significantly faster on a lot of threads.

svchb commented 3 days ago

See our statement of need so we basically want competing things but in the end this means to me we want to have: 1) a high-level API which hides unnecessary implementation details for new users 2) an API flexible enough to set-up most details without touching the source code