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

Dev/coordinate transforms #45

Merged
merged 43 commits into from
Jan 22, 2024
Merged

Dev/coordinate transforms #45

merged 43 commits into from
Jan 22, 2024

Conversation

austinschneider
Copy link
Collaborator

@austinschneider austinschneider commented Jan 16, 2024

In this PR we are trying to more strictly enforce which coordinate system is used in which part of the code. The main upshot is that coordinate transformation bugs in the distributions project will be much much much harder to make.

We are going about this by having the "Earth coordinates" only be used internally by the DetectorModel class.

This is implemented in two ways:

  1. The public interface of the DetectorModel class only uses the "Detector coordinates", with few exceptions.
  2. All methods of the DetectorModel class that accept coordinates of some form use named "strong types" (implemented with fluent::NamedType<T>) for dispatch, so that the particular method being called is clear at the invocation.

There are three exceptions to this division of the coordinate systems:

  1. The IntersectionList still contains "Earth coordinates"
  2. The Path class internally uses "Earth coordinates"
  3. The DetectorModel has methods for setting and getting the detector origin, which by construction must be in "Earth coordinates"

The first two exceptions exist so that we can avoid copious coordinate transformations in regular usage of the Path and DetectorModel classes, and are fine since we do not expect users to ever interact with the IntersectionsList internals. A consequence of this is that the SamplePairProduction method of the Injector class had to be removed (which was going to happen for other reasons in any case).

Ideally we do not want users on the python side to have to think about two coordinate systems, so the pybindings will be changed to only use the parts of the interface that accept DetectorPositions and DetectorDirections (with the exception of getting or setting the detector origin), and to perform the conversion from Vector3D to these tagged types automatically.

This PR may also solve the issue presented in #44
I have already come across several instances where the coordinate transformations were not correctly applied within the distributions project.

@austinschneider austinschneider added bug Something isn't working enhancement New feature or request labels Jan 16, 2024
@austinschneider austinschneider self-assigned this Jan 16, 2024
@austinschneider austinschneider marked this pull request as ready for review January 18, 2024 23:35
@austinschneider
Copy link
Collaborator Author

The latest commits fix an issue in the DIS cross section that was causing generated events to look like they fell below the Q^2 threshold.

@austinschneider
Copy link
Collaborator Author

This should now sort out all the issues I was having with Alex's ATLAS example script.

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 last commits get the DarkNews examples working again (as in, they generate events and there don't seem to be any coordinate system mismatches). Borrows some commits from the feature/examples branch. I think this branch is now ready to merge into main

@austinschneider austinschneider merged commit 3136b1d into main Jan 22, 2024
14 checks passed
@austinschneider austinschneider deleted the dev/coordinate_transforms 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.

Weights being inf when detector is translated and rotated within the PREM earth model
2 participants