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

Use default bounding box for FullGridCellList #665

Open
efaulhaber opened this issue Nov 14, 2024 · 4 comments
Open

Use default bounding box for FullGridCellList #665

efaulhaber opened this issue Nov 14, 2024 · 4 comments

Comments

@efaulhaber
Copy link
Member

As suggested in #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
Copy link
Collaborator

svchb commented Nov 14, 2024

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

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

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

svchb commented Nov 14, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants