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

Fiducial volume coordaintes #63

Merged
merged 4 commits into from
Mar 6, 2024
Merged

Fiducial volume coordaintes #63

merged 4 commits into from
Mar 6, 2024

Conversation

austinschneider
Copy link
Collaborator

@austinschneider austinschneider commented Mar 6, 2024

This PR aims to define the fiducial volume in detector coordinates within the SecondaryBoundedVertexDistribution.

Using geometry coordinates would have caused a weighting inconsistency when weighting with a geometry that has a different definition for detector coordinates than the one used for injection.

This PR also factorizes the logic that loads geometry files so that it can be used to load the fiducial volume, fixes the rotations used in the Geometry <--> Detector coordinate transforms, and fixes some issues with the create() methods.

… that allows coordinate choice, defaults to detector coords, and returns Geometry object in detector coords. Revert changes to SecondaryBoundedVertexDistribution
@austinschneider austinschneider added bug Something isn't working enhancement New feature or request labels Mar 6, 2024
@austinschneider austinschneider self-assigned this Mar 6, 2024
@austinschneider austinschneider marked this pull request as ready for review March 6, 2024 16:11
Copy link
Collaborator

@nickkamp1 nickkamp1 left a comment

Choose a reason for hiding this comment

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

The code changes all look good here. I'll test that the DN examples still work and give the correct output

@austinschneider
Copy link
Collaborator Author

austinschneider commented Mar 6, 2024

@nickkamp1 you might want to grab that last commit before you start testing. It looks like I had only reverted changes to the sampling method and hadn't done the same for GenerationProbability etc.

This obviously caused some issues when trying to do weighting in the CCM example

@nickkamp1
Copy link
Collaborator

@nickkamp1 you might want to grab that last commit before you start testing. It looks like I had only reverted changes to the sampling method and hadn't done the same for GenerationProbability etc.

This obviously caused some issues when trying to do weighting in the CCM example

Gotcha, tested with the latest commit. There was one remaining issue with the "in_fiducial" flag computed in LIController. This commit calculates that flag consistently with the behavior you implemented in this PR. I think this is good to merge now, will approve

@nickkamp1 nickkamp1 merged commit ebc150f into dev/examples Mar 6, 2024
6 checks passed
@austinschneider austinschneider deleted the dev/fiducial branch October 18, 2024 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants