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

feat: multidimensional, placeable, memory serializable grid #290

Merged
merged 5 commits into from
Oct 10, 2022

Conversation

niermann999
Copy link
Contributor

@niermann999 niermann999 commented Aug 17, 2022

Early draft for a grid implementation that addresses some issues found in the local navigation (grid navigation must be possible without reference surface)

  • 3D grids (done)
  • add grid local coordinates for a global to local transform (done)
  • have standalone grids and grids in detector 'grid collections' (capability is there, will be tested next PR)
  • implement populators and serializers (done)

This grid has a regular_attach_populator which keeps the same number of elements in every bin entry, since the grids are likely quite regular in a first implementation of the local navigation. An irregular_attach_populator using a jagged memory layout can be added later. A more elaborate iterator implementation that can resolve multiple range iteration (neighborhood -> multi-axis ranges -> bin entries). will be added with the 'ranges' PR #303 .

@niermann999 niermann999 marked this pull request as ready for review August 23, 2022 14:03
@niermann999 niermann999 requested a review from beomki-yeo August 23, 2022 14:04
@niermann999
Copy link
Contributor Author

Needs to be rebased on the latest additions to algebra-plugins, so that the global to local transformation also works for 3D cylinder and cuboid grids

@beomki-yeo
Copy link
Collaborator

grid navigation must be possible without reference surface

Why should it be possible without the reference surface? (BTW, what is the reference surface?) I want to understand the limitation of the current grid setup.

@asalzburger
Copy link
Contributor

grid navigation must be possible without reference surface

Why should it be possible without the reference surface? (BTW, what is the reference surface?) I want to understand the limitation of the current grid setup.

We need to be able to start on random points within the detector, i.e. in some random volume you need to be able to find the next candidate surfaces, there's no guarantee that you are on a surface.

@niermann999
Copy link
Contributor Author

grid navigation must be possible without reference surface

Why should it be possible without the reference surface? (BTW, what is the reference surface?) I want to understand the limitation of the current grid setup.

The grid has two axes, so two dimensions, which span a surface with local coordinates that are determined by the grid axes (i.e. polar on a disk, cylindrical on a cylinder etc.). The initial idea was to use the portal surfaces to transform the track position into a local position on the portal, which one could then also use to make the bin lookup on the axes. This becomes complicated, depending on whether you enter through a disk or a cylinder portal. Furthermore, the volumes are three dimensional, and once you move away from a portal surface, there is no global-to-local transform available anymore with which you could transform the track position into a 'grid local' point. Mainly, we need a global to local transform that comes with the grid itself.
It might not be immediately necessary for the current local navigation to have three-dimensional grids, but once we have volumes in volumes, I guess, we also need more than one bin in r in order to hit the right portals.

@beomki-yeo
Copy link
Collaborator

OK I understand the motivation. Someday we will need to remove the original grid2 after adapting traccc seed finding to the new grid. Would it be okay for you to rebase this PR once #291 gets merged?

@niermann999
Copy link
Contributor Author

OK I understand the motivation. Someday we will need to remove the original grid2 after adapting traccc seed finding to the new grid. Would it be okay for you to rebase this PR once #291 gets merged?

I must admit, I haven't looked at #291 in detail, yet, but in general I think it's a good idea to get that in first.

We need an attach populator with different bin capacities, though, before this could be used in traccc, right?

detector grids inside (Only works on host so far). Apart from that,
a brute force surface finder is also added. All can be accessed with
custom type IDs, the same way as it is implemented for the masks.
The surface finders can be called from the navigator using a
neighborhood_kernel, but for now they always return the complete
volume range, until the zone function of the grid is properly adapted.
The grids are built in the toy geometry in dedicated functions and
the detector now uses the custom bin_association to fill surfaces
into grids. For now, the grid building is commented out until we have
a working implementation of a grid container in a subsequent PR.

Further small changes:

 - The previous surface_finder class is no longer used in favour of
   the tuple_array_container. All typedefs are done by the
   type_registry.
 - The type ID enums are no longer implicitly convertible to an index
   for type safety, but can be converted, if necessary.
 - Where possible, all code that contains 'unrolling' has been put
   into functors and is called in the tuple_container.
 - The volume index is now fixed to dindex, since this is unlikely to
   change.
 - The surface finder link has been removed from the masks, since it
   is now part of the volume class.
 - Some minor refactoring (e.g. the edge types are renamed to
   volume_link for clarity).
 - More elaborate comments on some geometry classes and their
   template arguments.
 - For some reason, the usage of the detector memory resource in the
   navigator buffer broke, so they get an external resource now.
navigation (grid navigation must be possible without reference
surface). This PR provides:

 - 3D grids
 - Grid local coordinates for a global to local transform
 - Memory serialization: standalone grids and grids in grid collections
 - Implement populators, serializers and unittests, except for
   irregular attach populator.

Due to the 'operator==' not being a __device__ function, the
complete and attach populator unittests cannot pass the CI.

The PR also splits the geometric shape information (cylinder,
rectangle etc) from the navigation linking and boundary values, so
that they can be reused for the grid geometry definition. With that
comes the stringent use of local coordinates in the intersections
and some refactoring (renaming etc).
@niermann999 niermann999 added enhancement New feature or request refactor refactoring the current codes labels Oct 8, 2022
@niermann999
Copy link
Contributor Author

There are a few last problems with this PR that I will address later (see issues #310 and in algebra-plugins). Otherwise, I think this is good to go

@beomki-yeo
Copy link
Collaborator

Line coordinate part looks OK. Let's merge this in

@beomki-yeo beomki-yeo merged commit 88e3f26 into acts-project:main Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor refactoring the current codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants