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

Add distance to surface tests #224

Merged
merged 5 commits into from
Jan 15, 2025
Merged

Conversation

EricMEsch
Copy link
Collaborator

@EricMEsch EricMEsch commented Jan 10, 2025

Generates a dummy gdml volume consisting of 4 detectors (one of each type) and simulates 1 eV electrons sampled within those volumes. Then checks if the distance output from Geant4 agrees with the calculation from legendhpges. Currently the tolerance is implemented as default 1 nm. Additionally produces a plot that shows the hit profile for each detector.

Regarding the runtime: the 3 tests added here take combined ~ 10 secs to run on my machine, with all of the now 48 tests (excluding the vis tests) taking ~ 80 secs

Related #223

Edit: Maybe someone have a look over my python code. I copied a lot of stuff from Tobys implementation, but i also changed a lot. I am not that familiar with python so i might have done something in an overcomplicated way



# lar
lar_s = pg4.geant4.solid.Tubs(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lar can be removed from this GDML, but then change the mother volume for HPGe to be the world not lar.

detectors = ["V", "P", "B", "C"]


n = 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these lines are not needed in this case. They only create a macro to simulate in the LAr cylinders.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do kinda need both lines. The list of detectors is so i can just loop through all 4 types (although i can omit the detectors variable by just directly inserting the list into the enumerator).

And i need the n to keep track of the unique detector IDs to also create the detectors-fake.mac to register the detectors.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I selected the wrong lines. I meant 70-77 where we create the LAr cylinder information (lines)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, you are right. I dont know how i deleted the write statements below but not that^^

@tdixon97
Copy link
Collaborator

This looks a great test.
I think the threshold could be a bit smaller but also 1nm sounds fine.
Some lines of python code can be removed I added comments.

@tdixon97
Copy link
Collaborator

The test fails in CI:

AttributeError: module 'legendhpges' has no attribute 'draw

I have found this before, I think you have to import directly the hpges.draw submodule.`

@EricMEsch
Copy link
Collaborator Author

EricMEsch commented Jan 10, 2025

The test failed because the pre-commit hooks (namely ruff apparently) removed the from legendhpges import draw import due to "unused import" 🤦. I just changed the line to directly use that submodule now.

@tdixon97
Copy link
Collaborator

One other idea.
When I was writing the distance calculations in legendhpges I was a bit concerned we might have numerical issues if the global coordinates are large.
What about having a second test where we rerun the simulation but with the largest global offset we might have in eg. L-1000, eg. 5 m (is it reasonable?) and check everything still agrees?

@EricMEsch
Copy link
Collaborator Author

EricMEsch commented Jan 10, 2025

I just did some tests:

I think the threshold could be a bit smaller but also 1nm sounds fine.

I could reduce it. The distance in the output file is saved as a double in meters. For double we expect an accuracy of approximately 16 decimals, so we could even go to fm accuracy (although i am not sure of the accuracy in legendhpges, but my tests confirmed that they agree on 1e-15 meters tolerance).

The offset of possibly 5 meters should not be relevant.

@tdixon97
Copy link
Collaborator

tdixon97 commented Jan 10, 2025

Yeah I'm worried about the global offset more for the distances from legendhpges, which is based on the global positions. These distances are also validated by this test.
If we find there is an issue with this we might want to switch to storing the position as a local position in the remage output. What do you think @gipert ? Maybe local positions will be smaller and you can always reconstruct the global one from the GDML.

@EricMEsch
Copy link
Collaborator Author

Ah, i just realized that of course in geant4 we do encounter the same issue. As we do as well use the global coordinates first to then transform them into the local coordinates. And if we lost accuracy, we can not get it magically back using the transform. So forget my previous example^^

In Geant4 doubles are used, so the accuracy would roughly be (largest offset) * 1e-16 which should be fine for our setup

@EricMEsch EricMEsch added the validation Validation and tests label Jan 13, 2025
@gipert gipert linked an issue Jan 15, 2025 that may be closed by this pull request
@gipert gipert merged commit b2766c7 into legend-exp:main Jan 15, 2025
5 checks passed
@gipert
Copy link
Member

gipert commented Jan 15, 2025

Awesome, thanks Eric.

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

Successfully merging this pull request may close these issues.

Tests for DistanceToSurface
3 participants