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

Generic surface sampler #226

Merged
merged 28 commits into from
Jan 16, 2025
Merged

Generic surface sampler #226

merged 28 commits into from
Jan 16, 2025

Conversation

tdixon97
Copy link
Collaborator

@tdixon97 tdixon97 commented Jan 13, 2025

This implements the generic surface sampling following the algorithm of @jasondet et. al's paper [link].
No geantino are used instead only the methods G4VSolid::DistanceToIn(G4ThreeVector &p,G4ThreeVector &v) and DistanceToOut are needed. These essentially track the distance to the next surface along a line and so can give us the number of intersections.
I started on testing.

  • unit tests (of the C++ code) for the basic methods (this could be expanded to more types of volume),
  • some visualisation (it was tricky to get this to work well), for different simple shapes,
  • also some plots in python of the vertices and a check of selecting points on each surface and checking the fraction is equal to the ratio of surface areas,
    The last test is implemented but for some reason fails for the Union of 2 G4Box, in addition I find some generated points not on the surface, I will investigate.
    Update : This was related to the origin of the bounding sphere which wasn't accounted for - is now fixed.

Once we have this sorted we should think how to speed it up for difficult to sample solids, one idea is to check whether its impossible for a direction vector to intersect with the solid (eg. it points the wrong way) and then avoid tracking.

@tdixon97 tdixon97 requested a review from gipert January 13, 2025 00:26
@gipert gipert marked this pull request as draft January 13, 2025 09:28
@gipert gipert added enhancement New feature or request generators Event generators labels Jan 13, 2025
@gipert gipert linked an issue Jan 13, 2025 that may be closed by this pull request
@gipert
Copy link
Member

gipert commented Jan 13, 2025

Should we start adding some docstrings to the important functions? Can you give it a try? https://www.doxygen.nl/manual/docblocks.html

Please also add a link to the paper.

src/RMGVertexConfinement.cc Outdated Show resolved Hide resolved
src/RMGVertexConfinement.cc Outdated Show resolved Hide resolved
src/RMGVertexConfinement.cc Outdated Show resolved Hide resolved
src/RMGVertexConfinement.cc Outdated Show resolved Hide resolved
@tdixon97
Copy link
Collaborator Author

Should we start adding some docstrings to the important functions? Can you give it a try? https://www.doxygen.nl/manual/docblocks.html

Please also add a link to the paper.

I agree but there seems to be many options, maybe lets pick one ?

@tdixon97
Copy link
Collaborator Author

The number of PDF produced by the tests is now large. I think we should consider how to organise better the various confinement tests but also perhaps a strategy to merge the figures into one document?

@jasondet
Copy link

jasondet commented Jan 14, 2025 via email

@gipert
Copy link
Member

gipert commented Jan 14, 2025

The number of PDF produced by the tests is now large. I think we should consider how to organise better the various confinement tests but also perhaps a strategy to merge the figures into one document?

Yes, please feel free to reorganize things. If you come up with a proposal on how to merge things, I can implement something quick

@gipert
Copy link
Member

gipert commented Jan 14, 2025

I agree but there seems to be many options, maybe lets pick one ?

I like the first one:

/**
 *
 */

@tdixon97
Copy link
Collaborator Author

tdixon97 commented Jan 14, 2025

Well I think it would also be good to have some headings. And maybe also some text / captions could be included (eg. for geant4 vis where I havent been able to add text directly)

@tdixon97 tdixon97 marked this pull request as ready for review January 14, 2025 12:00
@tdixon97
Copy link
Collaborator Author

@jasondet Your algorithm generates N intersections and then picks one (or zero).
I understand that formally this keeps the sampled points independent. But I wonder if its really necessary for our applications.
In particular, this is a bit challenging since the maximum number of intersections has to be known in advance.

@tdixon97 tdixon97 marked this pull request as draft January 14, 2025 12:17
@tdixon97
Copy link
Collaborator Author

My geant4 visualisations don't work properly in CI, fine locally...

Co-authored-by: Manuel Huber <ManuelHu@users.noreply.github.com>
@jasondet
Copy link

@tdixon97 it is indeed probably not necessary to pick one of the N crossings at random -- at high stats you still get distribution that is essentially uniform over any surface, the only price you pay is that some consecutive points have some hint of a correlation (they lie along the same ray). That kind of correlation is probably irrelevant for our application (rad sims).

However inputting N_max is not difficult. We would just set it to something that seemed reasonable (like 16 or 32 or something) and then bumped it up by a factor of 2 and re-ran if we ever saw the error message that it was too small. Then we didn't have to worry about arguing whether the faint correlation was okay or not. So we opted to do that.

@tdixon97
Copy link
Collaborator Author

@tdixon97 it is indeed probably not necessary to pick one of the N crossings at random -- at high stats you still get distribution that is essentially uniform over any surface, the only price you pay is that some consecutive points have some hint of a correlation (they lie along the same ray). That kind of correlation is probably irrelevant for our application (rad sims).

However inputting N_max is not difficult. We would just set it to something that seemed reasonable (like 16 or 32 or something) and then bumped it up by a factor of 2 and re-ran if we ever saw the error message that it was too small. Then we didn't have to worry about arguing whether the faint correlation was okay or not. So we opted to do that.

I wrote our implementation so only one G4VSolid is ever sampled at a time Nmax can probably be usually something around 6 in our case (so less bad). However, since we weight the intersections by N/Nmax this still means we though away a lot of points and so are also using a lot of calls to GetRandom(). I guess that the vertex confinement is usually so fast this doesn't matter. But there are cases where it might...
I agree the correlation is clearly there but probably minimal.

Testing/Temporary/LastTest.log Outdated Show resolved Hide resolved
docs/developer.md Show resolved Hide resolved
include/RMGVertexConfinement.hh Outdated Show resolved Hide resolved
include/RMGVertexConfinement.hh Outdated Show resolved Hide resolved
include/RMGVertexConfinement.hh Outdated Show resolved Hide resolved
include/RMGVertexConfinement.hh Outdated Show resolved Hide resolved
src/RMGVertexConfinement.cc Show resolved Hide resolved
src/RMGVertexConfinement.cc Outdated Show resolved Hide resolved
tests/tex/CMakeLists.txt Outdated Show resolved Hide resolved
tests/tex/validation-report.tex Outdated Show resolved Hide resolved
@tdixon97
Copy link
Collaborator Author

Should now be ready @gipert

@tdixon97 tdixon97 marked this pull request as ready for review January 16, 2025 11:57
@gipert gipert merged commit 6647bce into legend-exp:main Jan 16, 2025
5 checks passed
@tdixon97 tdixon97 mentioned this pull request Jan 16, 2025
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request generators Event generators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement sampling from complex surfaces and intersections
4 participants