-
Notifications
You must be signed in to change notification settings - Fork 37
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
update TrackerHitPlane doc #160
Open
andiwand
wants to merge
1
commit into
key4hep:main
Choose a base branch
from
andiwand:patch-1
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I understand it right U defines the X axis and V defines the Y axis for
position
. the Z component will be ignored then?what is also not clear to me: why is there a
du
anddv
if there is also acovMatrix
that describes the covariance ofposition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class has been copied from LCIO - see here for a more complete documentation: https://ilcsoft.desy.de/LCIO/current/doc/doxygen_api/html/classEVENT_1_1TrackerHitPlane.html.
The measurement directions
u,v
are the two directions of the measurement surface in global (polar) coordinates. Thedu, dv
are the resolutions along these directions, i.e. the point resolution. Sou,v
are not really X and Y.The
covMatrix
is given in the global cartesian coordinate and can be given for all types ofTrackerHits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you @gaede ! will the
position
contain then theU
V
values in form ofX
Y
? this is where I get confused because you saidcovMatrix
will be in global cartesian coordinatesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No,
u,v
really should be in(theta, phi)
, i.e spherical (and not polar coordinates). The global cartesian covariance matrix is a left-over from a time when there was only the genericTrackerHit
in LCIO.One could think about dropping this from EDM4hep...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but
U
andV
will only span the measurement direction for the plane, right? as you said in(theta, phi)
global spherical coordinates.but where do you then store the actual local measurement values for
U
andV
? will they go into theposition
'sX
andY
? and what happens to theZ
component?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, the documentation needs to be cleaned up and extended.
Not sure I understand your comment about local uncertainties and their correlation being dropped. There should be none - at least that is what is assumed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can recover the off-diagonal covariance entries from the global position and global covariance (as well as
du,dv
I suppose), but they're not directly stored (whiledu,dv
are). That was all.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I understand it
du
anddv
is basically the pixel size in each direction? so basically just a property of the module. covariance comes into play after we actually measure somethinginitially I thought you would store global and local coordinates. but actually there are only global coords and the information required to turn them into local coords, right? I just wondered why this is duplicated of each measurement instead of relying on a geometry definition like DD4hep (or DDRec? not sure) or having a surface object in EDM4hep where you can point to with a relation.
the LCIO doc is much better. I guess it can be just copied over. especially the "x-y plane" and "z axis" thing confused me a lot in the current doc in EDM4hep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I actually thought it was the actual diagonal covariance matrix elements of the measurement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually you cannot calculate the local coordinates because it would require the global cartesian coordinates of the origin of the detector module that was hit, right?