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

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft

Generic surface sampler #226

wants to merge 21 commits into from

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:

/**
 *
 */

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

tdixon97 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

What about making a LaTex document with all the plots, although then of course we need some compiler in the container, and to constantly update that template. But I think it would be very nice to have the plots organised by test type.

@gipert
Copy link
Member

gipert commented Jan 14, 2025

Depends on how you want this report document to look like. If it's just a collection of plots then we can do something more simple than LaTeX...

@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...

@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.

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